Hi JB and all, I'm personally fine with shorter review cycles. If other committers agree that with two approvals minimum we can have shorter review cycles, I'd be happy to make that adjustment.
>From my side, I'm trying to establish guidelines that will help committers gradually reach a state of mind when reviewers are aware of potential implications of code changes in certain areas and take that into consideration naturally. Essentially, if we can avoid code change "surprises", I think actual time a PR stays in review should not matter. Perhaps I'm not totally clear on that :) so I'm willing to have a few iterations on this PR with feedback from committers. Re: the 72 hour wait time, I thought it would align with the 72 hour vote duration minimum at the ASF, but as I said above, faster reviews should be fine. I certainly did not mean my proposal to be treated as "strict" rules, just as a basis for mutual expectations. Here's an updated version, which I hope can help us reach a common understanding. - All PRs need to be approved by at least two committers. - REST API changes are to be discussed on `dev` in all cases. (I believe this is the only strict rule and the community is aware of this... just adding for clarity) - If the change is small and relatively isolated, the change can be merged immediately upon approval. - If the change can reasonably be expected to have non-trivial effects in runtime or in downstream projects, extra review time is recommended so that reviewers can start discussions on the PR if they have specific feedback. The general recommendation for starting discussions on non-trivial PRs is 3 working days. This waiting period is both a courtesy to reviewers who might have busy schedules and also a request for goodwill in providing timely feedback. Once discussions have consensus and the PR is approved, it can be merged. - If a concern is missed during review, let's fix forward on `main`. We'll learn from this and take it into account in subsequent reviews (common sense). WDYT? Cheers, Dmitri. On Thu, Nov 27, 2025 at 1:00 PM Jean-Baptiste Onofré <[email protected]> wrote: > Hi Dmitri, > > Even though I am not a big fan of strict rules or guidelines (I trust > people :)), your proposal seems reasonable to me. > > I would suggest one addition: requiring a minimum of two approvals for > a PR to be merged. This would also serve as a mechanism to naturally > introduce a waiting period for reviews (and thus, the suggested "x > days"). > > Regards, > JB > > On Thu, Nov 27, 2025 at 6:27 PM Dmitri Bourlatchkov <[email protected]> > wrote: > > > > Thanks for the feedback, Adam! > > > > I'd also appreciate more active engagement from Polaris committers, since > > they are the ones who are affected by this change in guidelines :) All > > opinions are welcome. > > > > Thanks, > > Dmitri. > > > > On Fri, Nov 21, 2025 at 11:03 AM Adam Christian < > > [email protected]> wrote: > > > > > Personally, I like having guidelines for the implicit rules that we > already > > > know about and, then, growing as the project grows and as we learn from > > > users/contributors. > > > > > > For example, one of my favorite community learnings has been around > > > authorization. Polaris came with native RBAC, but we have seen several > > > contributors want to extend this functionality - I'm thinking of the > OPA > > > work as one recent example. Eventually, I would expect that this would > > > morph into an explicit SPI with explicit guidelines around > contribution & > > > evolution. > > > > > > So, I think the PR guidelines you have written are good and we can > iterate > > > as we learn. This way, we aren't trying to solve all of our problems at > > > once, but remain flexible to how the project grows. > > > > > > Go community, > > > > > > Adam > > > > > > On Fri, Nov 21, 2025 at 9:18 AM Dmitri Bourlatchkov <[email protected]> > > > wrote: > > > > > > > Hi All, > > > > > > > > It looks like this PR attracted somewhat different opinions in terms > of > > > > what is a significant change, what is not and when to wait. Let's > discuss > > > > this on this thread before revisioning the PR. > > > > > > > > From my POV, having strict rules is not practical at this stage of > the > > > > project because: > > > > > > > > * API/SPI is not well-defined > > > > * The codebase evolves quickly, so it is hard to predict downstream > > > ripple > > > > effects. > > > > * Contributor interests are not equally distributed across the > codebase > > > > > > > > I'd propose a common sense approach where the author and reviewers > assess > > > > the impact to the best of their knowledge and then decide: > > > > > > > > 1) If the change is small and relatively isolated, the change can be > > > merged > > > > immediately. > > > > 2) If the change can reasonably be expected to have non-trivial > effects > > > in > > > > runtime or in downstream projects, wait 3 working days for other > > > reviewers > > > > to start discussions if necessary. > > > > 3) REST API changes are to be discussed on `dev` in all cases. > > > > 4) If a concern is missed during review, let's fix forward on `main`. > > > We'll > > > > learn from this and take it into account in subsequent reviews > (common > > > > sense). > > > > > > > > As the project matures and java API/SPI modules are more formally > > > defined, > > > > we can add more specific guidelines for them. > > > > > > > > WDYT? > > > > > > > > Thanks, > > > > Dmitri. > > > > > > > > On Mon, Nov 17, 2025 at 10:05 AM Dmitri Bourlatchkov < > [email protected]> > > > > wrote: > > > > > > > > > Hi All, > > > > > > > > > > I opened PR [3067] proposing a bit of clarification and more review > > > time > > > > > for more intrusive PRs. Please refer to GH for the example > language. > > > > > > > > > > Please comment about grammar / spelling in GH, but if you have > comments > > > > > about the subject matter, it might be best to use this email > thread for > > > > > discussing those. > > > > > > > > > > [3067] https://github.com/apache/polaris/pull/3067 > > > > > > > > > > Thanks, > > > > > Dmitri. > > > > > > > > > > > > >
