docs: adjust review time expectations#3067
Conversation
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
adnanhemani
left a comment
adnanhemani
left a comment
There was a problem hiding this comment.
I might recommend four or five working days if the change is quite intrusive (based on my personal experience) - but we can start here!
CONTRIBUTING.md
Outdated
| * Do not re-create a pull-request for the same change. Use one Pull Request related to the same change(s). The purpose here is to keep the history and all comments in the Pull Request. | ||
| * Consider open questions and concerns in all comments of your Pull Request, provide replies and resolve addressed comments, if those don't serve reference purposes. If a comment doesn't contain `nit`, `minor`, or `not a blocker` mention, please provide feedback to the comment before merging. | ||
| * Give time for review. For instance two working days is a good base to get first reviews and comments. | ||
| * Give time for review. For instance two working days is a good base to get first reviews and comments for changes that are not likely to affect downstream projects. For changes that touch core interfaces and behaviours three working days would be advisable. |
There was a problem hiding this comment.
We discussed what we consider integration points here : https://lists.apache.org/thread/0nj24zro7kyctqfnlml08ppo7zs9xcqs, wondering if we should link it here or be a bit precise on expectation on what interface we should give additional time to review
There was a problem hiding this comment.
Can we add that SPIs/APIs changes, as well as structural changes like a Java module added/removed?
There was a problem hiding this comment.
SPI/API are not well-defined in Polaris now, IMHO. The discussion linked above is a good starting point, but I do not think we completely ironed it out yet 😅 Let's continue the dev discussion by email and update guidelines when we have a solid definition of SPI/API.
For now, I believe it has to remain a bit vague and applied with common sense.
There was a problem hiding this comment.
It's true that some SPIs/APIs in Polaris are still not clearly defined. That said, we do have well-defined ones, for example, the REST Spec.
Also, I'd suggest that structural changes, such as adding or removing Java modules, should probably allow for a longer review window.
There was a problem hiding this comment.
Let's discuss this on dev
dimas-b
force-pushed
the
adjust-contribution-doc
branch
from
c710f8d to
edd4be0
Compare
|
|
||
| * Smaller changes that are not likely to affect end users or downstream projects can be merged on | ||
| immediately on approval. If concerns are flagged later, they are to be addressed in follow-up PRs. | ||
| * Waiting at least two working days is recommented (even if the PR has approvals from some reviewers) for: |
There was a problem hiding this comment.
Nit (spelling): recommented -> recommended
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| * Waiting at least two working days is recommented (even if the PR has approvals from some reviewers) for: | ||
| - Refactorings that may affect downstream projects. | ||
| - Adding new features even if they are "off" by default. | ||
| * Three working days are recommended in these cases (even if the PR has approvals from some reviewers): |
There was a problem hiding this comment.
I like @singhpk234 's idea of having two reviewers for critical/big changes described here.
There was a problem hiding this comment.
This PR is in draft awaiting consensus on dev ML discussions. These are the threads, that I think are related:
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
7 participants
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)