Zooming out a bit, I think Accord is the first large body of work we've done post introduction of the CEP system with multiple people collaborating on a feature branch like this. This discussion seems to have surfaced a few sentiments:
1. Some contributors seem to feel that work on a feature branch doesn't have the same inherent visibility as work on trunk 2. There's a lack of clarity on our review process when it comes to significant (in either time or size) rebases 3. We might be treating Ninja commits a bit differently on a feature branch than trunk, which intersects with 1 and 2 My personal opinions are: I disagree with 1 - it simply takes the added effort of actively following that branch and respective JIRAs if you're interested. I think having a feature branch in the ASF git repo w/commits and JIRAs tracking that work is perfectly fine, and the existing bar (2 committers +1, green tests before merge to trunk) when applied to a feature branch is also not just well within the "letter of the law" on the project but also logically sufficient to retain our bar of quality and stability. For 2 (reviews required after rebase) I don't think we should over-prescribe process on this. If all tests are green pre-rebase, and all tests are green post-rebase, and a committer is confident they didn't materially modify the functioning of the logical flow or data structures of their code during a rebase, I don't see there being any value added by adding another review based on those grounds. If the subtext is actually that some folks feel we need a discussion about whether we should have a different bar for review on CEP feature branches (3 committers? 1+ pmc members? more diversity in reviewers or committers as measured by some as yet unspoken metric), perhaps we could have that discussion. FWIW I'm against changes there as well; we all wear our Apache Hats here, and if the debate is between work like this happening in a feature branch affording contributors increased efficiency and locality vs. all that happening on trunk and repeatedly colliding with everyone everywhere, feature branches are a clear win IMO. And for 3 - I think we've all broadly agreed we shouldn't ninja commit unless it's a comment fix, typo, forgotten git add, or something along those lines. For any commit that doesn't qualify it should go through the review process. And a final note - Ekaterina alluded to something valuable in her email earlier in this thread. I think having a "confirm green on all the test suites that are green on merge target" bar for large feature branches (rather than strictly the "pre-commit subset") before merge makes a lot of sense. On Tue, Jan 24, 2023, at 1:41 PM, Caleb Rackliffe wrote: > Just FYI, I'm going to be posting a Jira (which will have some dependencies > as well) to track this merge, hopefully some time today... > > On Tue, Jan 24, 2023 at 12:26 PM Ekaterina Dimitrova <e.dimitr...@gmail.com> > wrote: >> I actually see people all the time making a final check before merge as part >> of the review. And I personally see it only as a benefit when it comes to >> serious things like Accord, as an example. Even as a help for the author who >> is overwhelmed with the big amount of work already done - someone to do >> quick last round of review. Team work after all. >> >> Easy rebase - those are great news. I guess any merge conflicts that were >> solved will be documented and confirmed with reviewers before merge on the >> ticket where the final CI push will be posted. I also assumed that even >> without direct conflicts a check that there is no contradiction with any >> post-September commits is done as part of the rebase. (Just adding here for >> completeness) >> >> One thing that I wanted to ask for is when you push to CI, you or whoever >> does it, to approve all jobs. Currently we have pre-approved the minimum >> required jobs in the pre-commit workflow. I think in this case with a big >> work approving all jobs in CircleCI will be of benefit. (I also do it for >> bigger bodies of work to be on the safe side) Just pointing in case you use >> a script or something to push only the pre-approved ones. Please ping me in >> Slack if It’s not clear what I mean, happy to help with that >> >> On Tue, 24 Jan 2023 at 11:52, Benedict <bened...@apache.org> wrote: >>> >>> Perhaps the disconnect is that folk assume a rebase will be difficult and >>> have many conflicts? >>> >>> We have introduced primarily new code with minimal integration points, so I >>> decided to test this. I managed to rebase locally in around five minutes; >>> mostly imports. This is less work than for a rebase of fairly typical >>> ticket of average complexity. >>> >>> Green CI is of course a requirement. There is, however, no good procedural >>> or technical justification for a special review of the rebase. >>> >>> Mick is encouraged to take a look at the code before and after rebase, and >>> will be afforded plenty of time to do so. But I will not gate merge on this >>> adhoc requirement. >>> >>> >>> >>> >>>> On 24 Jan 2023, at 15:40, Ekaterina Dimitrova <e.dimitr...@gmail.com> >>>> wrote: >>>> >>>> Hi everyone, >>>> I am excited to see this work merged. I noticed the branch is 395 commits >>>> behind trunk or not rebased since September last year. I think if Mick or >>>> anyone else wants to make a final pass after rebase happens and CI runs - >>>> this work can only benefit of that. Squash, rebase and full CI run green >>>> is the minimum that, if I read correctly the thread, we all agree on that >>>> part. >>>> I would say that CI and final check after a long rebase of a patch is a >>>> thing we actually do all the time even for small patches when we get back >>>> to our backlog of old patches. This doesn’t mean that the previous reviews >>>> are dismissed or people not trusted or anything like that. >>>> But considering the size and the importance of this work, I can really see >>>> only benefit of a final cross-check. >>>> As Henrik mentioned me, I am not sure I will have the chance to review >>>> this work any time soon (just setting the right expectations up front) but >>>> I see at least Mick already mentioning he would do it if there are no >>>> other volunteers. Now, whether it will be separate ticket or not, that is >>>> a different story. Aren’t the Accord tickets in an epic under which we can >>>> document the final rebase, CI runs, etc? >>>> >>>> On Tue, 24 Jan 2023 at 9:40, Henrik Ingo <henrik.i...@datastax.com> wrote: >>>>> When was the last time the feature branch was rebased? Assuming it's a >>>>> while back and the delta is significant, surely it's normal process to >>>>> first rebase, run tests, and then present the branch for review? >>>>> >>>>> To answer your question: The effect of the rebase is then either baked >>>>> into the original commits (which I personally dislike), or you can also >>>>> have the rebase-induced changes as their own commits. (Which can get >>>>> tedious, but has the benefit of making explicit what was only a change >>>>> due to rebasing.) Depending on which approach you take when rebasing, a >>>>> reviewer would then review accordingly. >>>>> >>>>> henrik >>>>> >>>>> On Tue, Jan 24, 2023 at 11:14 AM Benedict <bened...@apache.org> wrote: >>>>>> >>>>>> No, that is not the normal process. What is it you think you would be >>>>>> reviewing? There are no diffs produced as part of rebasing, and the >>>>>> purpose of review is to ensure code meets out standards, not that the >>>>>> committer is competent at rebasing or squashing. Nor are you familiar >>>>>> with the code as it was originally reviewed, so would have nothing to >>>>>> compare against. We expect a clean CI run, ordinarily, not an additional >>>>>> round of review. If we were to expect that, it would be by the original >>>>>> reviewer, not a third party, as they are the only ones able to judge the >>>>>> rebase efficiently. >>>>>> >>>>>> I would support investigating tooling to support reviewing rebases. I’m >>>>>> sure such tools and processes exist. But, we don’t have them today and >>>>>> it is not a normal part of the review process. If you want to modify, >>>>>> clarify or otherwise stipulate new standards or processes, I suggest a >>>>>> separate thread. >>>>>> >>>>>> > How will the existing tickets make it clear when and where their final >>>>>> > merge happened? >>>>>> >>>>>> By setting the release version and source control fields. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> On 24 Jan 2023, at 08:43, Mick Semb Wever <m...@apache.org> wrote: >>>>>>> >>>>>>> >>>>>>>> .... But it's not merge-than-review, because they've already been >>>>>>>> reviewed, before being merged to the feature branch, by committers >>>>>>>> (actually PMC members)? >>>>>>>> >>>>>>>> You want code that's been written by one PMC member and reviewed by 2 >>>>>>>> other PMC members to be put up for review by some random 4th party? >>>>>>>> For how long? >>>>>>> >>>>>>> >>>>>>> It is my hope that the work as-is is not being merged. That there is a >>>>>>> rebase and some trivial squashing to do. That deserves a quick check by >>>>>>> another. Ideally this would be one of the existing reviewers (but like >>>>>>> any other review step, no matter how short and trivial it is, that's >>>>>>> still an open process). I see others already doing this when rebasing >>>>>>> larger patches before the final merge. >>>>>>> >>>>>>> Will the branch be rebased and cleaned up? >>>>>>> How will the existing tickets make it clear when and where their final >>>>>>> merge happened? >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>>>> -- >>>>> >>>>> >>>>> >>>>> >>>>> *Henrik Ingo* >>>>> >>>>> *c*. +358 40 569 7354 >>>>> >>>>> *w*. _www.datastax.com_ >>>>> >>>>> __ <https://www.facebook.com/datastax> __ <https://twitter.com/datastax> >>>>> __ <https://www.linkedin.com/company/datastax/> __ >>>>> <https://github.com/datastax/> >>>>>
