andygrove opened a new pull request, #6030: URL: https://github.com/apache/datafusion-comet/pull/6030
## Which issue does this PR close? No issue. This is a process proposal, so the discussion belongs on the pull request itself. - [Rendered page — Review expectations](https://github.com/andygrove/datafusion-comet/blob/docs-review-expectations/docs/source/contributor-guide/contributing.md#review-expectations) - [Rendered page — Merging through the queue](https://github.com/andygrove/datafusion-comet/blob/docs-review-expectations/docs/source/contributor-guide/ci.md#merging-through-the-queue) ## Rationale for this change We have a convention that a non-trivial pull request stays open long enough for more than one maintainer to see it, and it is written down nowhere. I went looking for it and came up empty. `contributing.md` covers finding issues, reporting issues, Slack, and the community call, but has no review-process section at all. The nearest thing is one sentence in the CI guide — "Once a pull request is approved, a committer queues it with **Merge when ready**" — which describes only the mechanics, and immediately adds that the pull request's own checks do not have to be finished. Read literally, one approval at any hour is the whole bar. That reads differently depending on where you sit. The people who know a given area are spread across many time zones, and a change queued a few hours after it is opened has only been seen by whoever happened to be awake. For a typo that is fine. For a planner or serde change it means the reviewer who would have recognized the problem never got the chance, and the first anyone hears of it is a revert or a nightly failure. ## What changes are included in this PR? Documentation only. `contributing.md` gains a **Review expectations** section: leave a non-trivial pull request open for at least 24 hours after it is ready for review, even once it has an approval. It lists what counts as non-trivial (planner, serde, native operators, shims; defaults, config names, public APIs; new or major-version dependency bumps; performance work; anything under `dev/diffs/`) and what does not (docs, comments, test-only changes, routine bumps, and repairs to a red `main`, where waiting costs more than the review would catch). `ci.md` gains three sentences in **Merging through the queue** pointing at it, since that is the page a committer is reading when they reach for **Merge when ready**. Four choices in the draft that I would like opinions on: - The clock starts when the pull request is ready for review, not when it is opened, so a draft that sat for a week does not satisfy it without anyone having looked. - It is written as a convention, and the text says plainly that nothing enforces it. There is no required check, and the merge queue does not know how long a pull request has been open. A rule the tooling contradicts gets ignored wholesale, so I would rather state it accurately than imply a gate that does not exist. - Red `main` and broken-build repairs are exempt by name, so nobody has to argue the case mid-incident. - It does not require two approvals. It buys the opportunity for a second opinion without mandating one. If people would rather have an actual two-approval rule on the listed categories, that is a stronger policy and a different paragraph — happy to write it, but I did not want to assume it. 24 hours covers one full timezone cycle, which is the whole point of the number. If people would rather have something else, or would rather scope the list differently, say so. ## How are these changes tested? No tests — documentation only. `npx prettier "**/*.md"` reports both files unchanged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
