> I know that many of us review code after it has been merged, and what we have > been doing is filing follow up bugs/improvement requests as new work. I think this is key. The code on the feature branch matches the same bar of quality / process that's on trunk, and it's trivially easy to checkout at the merge SHA and review the code in an IDE even after merge, raising questions with someone if you have concerns. If we were talking about merging this feature branch a week before a GA I could see why there'd be concern but we have a lot of calendar runway here.
I'd expect modifications post merge to trunk to have the same inertia for/against them as modifications on a feature branch that have cleared our 2 committer bar for merge. > 1. Code must not be committed if a committer has requested reasonable time > to conduct a review I'm realizing in retrospect this leaves ambiguity around *where* the code is committed (i.e. trunk vs. feature branch). On the one hand we could say this code has already been reviewed and committed; the feature branch had the same bar as trunk. On the other hand you could say the intent of this agreement was to allow committers to inspect code *before it goes to trunk*, which would present a different set of constraints. My perspective is that of the former; I consider this code "already committed". This work was done on a formal feature branch w/JIRA tickets and feedback / review done on the PR's in the open on the feature branch by 2+ committers, so folks have had a reasonable time to engage with the process and conduct a review. I don't think right on the cusp of a ceremonial / procedural cutover from a feature branch to trunk is the right time, nor scope of work, to propose a blocking review based on this text. On Fri, Jan 27, 2023, at 3:10 PM, David Capwell wrote: >> I've learned that when I have defended the need (or right, if appealing to >> the Governance texts...) for contributors to be able to review a feature >> branch at the time it is merged to trunk - which for Accord is now - that a >> common reaction to this is that doing a review of Accord now might take >> months and would stall the Accord project for months if that is allowed. > > The way I have been reading this thread is not that “we don’t want the review > as it slows us down” but more “the process is X, why are you asking for Y?”. > I believe I can speak for everyone working on Accord, we all want more > reviewers and contributors! > > I think its fair to ask the question on if feature branches “should" have the > same process as non-feature branches, but since that has not been called out > and voted on they are expected to follow the same process; if people working > on feature branches have not been following the same process that is > currently an issue that needs to be addressed (In the case of Accord all > contributors are Committers or PMC and same process has been followed as > trunk). The project doesn’t have a good history (at least recent history) of > open development of features, most features are dumps from private sources, > so there are learning/growing pains as we try to develop new features in the > open. > >> On the other hand, I can think of many things that a pair of fresh eyes can >> do at this point in reasonable time, like days or a couple weeks. > I agree more eyes are better, but the conversation is on code that has > already merged. I know that many of us review code after it has been merged, > and what we have been doing is filing follow up bugs/improvement requests as > new work. A simple example of this was when the trie memtables patch landed > I reviewed code that was merged I think 2021 making memtables pluggable; I > found a bug in it, showed the bug existed, and worked with the authors to get > all this addressed. Reviews after the fact are fine, common, and welcome in > this project, but that has always sponsored new tickets and new work to > address the feedback. > > >> On Jan 27, 2023, at 7:39 AM, Henrik Ingo <henrik.i...@datastax.com> wrote: >> >> While the substance of the review discussion has moved to Jira, I wanted to >> come back here to clarify one thing: >> >> I've learned that when I have defended the need (or right, if appealing to >> the Governance texts...) for contributors to be able to review a feature >> branch at the time it is merged to trunk - which for Accord is now - that a >> common reaction to this is that doing a review of Accord now might take >> months and would stall the Accord project for months if that is allowed. >> >> So I just wanted to clarify I don't think that sounds "reasonable", as the >> word is used in the Governance wiki page. I agree that to engage in such >> level of review, it would have needed to happen earlier. On the other hand, >> I can think of many things that a pair of fresh eyes can do at this point in >> reasonable time, like days or a couple weeks. >> >> I spent 6 hours this week glancing over the 28k lines of code that would be >> added to C* codebase. I was able to form an opinion of the patch, have some >> fruitful off-list conversations with several people, and as a by-product >> apparently also caught some commented out code that possibly should be >> enabled before the merge. >> >> henrik >> >> On Wed, Jan 25, 2023 at 5:06 PM Henrik Ingo <henrik.i...@datastax.com> wrote: >>> Thanks Benedict >>> >>> For brevity I'll respond to your email, although indirectly this is also a >>> continuation of my debate with Josh: >>> >>> At least on my scorecard, one issue was raised regarding the actual code: >>> CASSANDRA-18193 Provide design and API documentation. Since the addition of >>> code comments also significantly impacts the ability of an outsider to >>> understand and review the code, I would then treat it as an unknown to say >>> how much else such a fresh review would uncover. >>> >>> By the way I would say the discussion about git submodules (and all the >>> other alternatives) in a broad sense was also a review'ish comment. >>> >>> That said, yes of course the expectation is that if the code has already >>> been reviewed, and by rather experienced Cassandra developers too, there >>> probably won't be many issues found, and there isn't a need for several >>> weeks of line by line re-review. >>> >>> As for the rebase, I think that somehow started dominating this discussion, >>> but in my view was never the only reason. For me this is primarily to >>> satisfy points 4 and 5 in the project governance, that everyone has had an >>> opportunity to review the code, for whatever reason they may wish to do so. >>> >>> I should say for those of us on the sidelines we certainly expected a >>> rebase catching up 6 months and ~500 commits to have more substantial >>> changes. Hearing that this is not the case is encouraging, as it also >>> suggests the changes to Cassandra code are less invasive than maybe I and >>> others had imagined. >>> >>> henrik >>> >>> On Wed, Jan 25, 2023 at 1:51 PM Benedict <bened...@apache.org> wrote: >>>> >>>>> contributors who didn't actively work on Accord, have assumed that they >>>>> will be invited to review now >>>> >>>> I may have missed it, but I have not seen anyone propose to substantively >>>> review the actual *work*, only the impact of rebasing. Which, honestly, >>>> there is plenty of time to do - the impact is essentially nil, and we >>>> aren’t planning to merge immediately. I will only not agree to an adhoc >>>> procedural change to prevent merge until this happens, as a matter of >>>> principle. >>>> >>>> However, I am very sympathetic to a desire to participate *substantively*. >>>> I think interested parties should have invested as the work progressed, >>>> but I *don’t* think it is unreasonable to ask for a *some* time prior to >>>> merge if this hasn’t happened. >>>> >>>> So, if you can adequately resource it, we can delay merging a while >>>> longer. I *want* your (constructive) participation. But, I have not seen >>>> anything to suggest this is even proposed, let alone realistic. >>>> >>>> There are currently five full time contributors participating in the >>>> Accord project, with cumulatively several person-years of work already >>>> accumulated. By the time even another month has passed, you will have >>>> another five person-months of work to catch-up on. Resourcing even a >>>> review effort to catch up with this is *non-trivial*, and for it to be a >>>> reasonable ask, you must credibly be able to keep up while making useful >>>> contributions. >>>> >>>>> After all, if it had been ready to merge to trunk already a year ago, why >>>>> wasn't it? >>>> >>>> The Cassandra integration has only existed since late last year, and was >>>> not merged earlier to avoid interfering with the effort to release 4.1. >>>> >>>>> One thing that I wanted to ask for is when you push to CI, you or whoever >>>>> does it, to approve all jobs. >>>> >>>> Thanks Ekaterina, we will be sure to fully qualify the CI result, and I >>>> will make sure we also run your flaky test runner on the newly introduced >>>> tests. >>>> >>>> >>>> >>>> >>>> >>>>> On 24 Jan 2023, at 21:42, Henrik Ingo <henrik.i...@datastax.com> wrote: >>>>> >>>>> Thanks Josh >>>>> >>>>> Since you mentioned the CEP process, I should also mention one sentiment >>>>> you omitted, but worth stating explicitly: >>>>> >>>>> 4. The CEP itself should not be renegotiated at this point. However, the >>>>> reviewers should rather focus on validating that the implementation >>>>> matches the CEP. (Or if not, that the deviation is of a good reason and >>>>> the reviewer agrees to approve it.) >>>>> >>>>> >>>>> Although I'm not personally full time working on either producing >>>>> Cassandra code or reviewing it, I'm going to spend one more email >>>>> defending your point #1, because I think your proposal would lead to a >>>>> lot of inefficiencies in the project, and that does happen to be my job >>>>> to care about: >>>>> >>>>> - Even if you could be right, from some point of view, it's nevertheless >>>>> the case that those contributors who didn't actively work on Accord, have >>>>> assumed that they will be invited to review now, when the code is about >>>>> to land in trunk. Not allowing that to happen would make them feel like >>>>> they weren't given the opportunity and that the process in Cassandra >>>>> Project Governance was bypassed. We can agree to work differently in the >>>>> future, but this is the reality now. >>>>> >>>>> - Although those who have collaborated on Accord testify that the code >>>>> is of the highest quality and ready to be merged to trunk, I don't think >>>>> that can be expected of every feature branch all the time. In fact, I'm >>>>> pretty sure the opposite must have been the case also for the Accord >>>>> branch at some point. After all, if it had been ready to merge to trunk >>>>> already a year ago, why wasn't it? It's kind of the point of using a >>>>> feature branch that the code in it is NOT ready to be merged yet. (For >>>>> example, the existing code might be of high quality, but the work is >>>>> incomplete, so it shouldn't be merged to trunk.) >>>>> >>>>> - Uncertainty: It's completely ok that some feature branches may be >>>>> abandoned without ever merging to trunk. Requiring the community (anyone >>>>> potentially interested, anyways) to review such code would obviously be a >>>>> waste of precious talent. >>>>> >>>>> - Time uncertainty: Also - and this is also true for Accord - it is >>>>> unknown when the merge will happen if it does. In the case of Accord it >>>>> is now over a year since the CEP was adopted. If I remember correctly an >>>>> initial target date for some kind of milestone may have been Summer of >>>>> 2022? Let's say someone in October 2021 was invested in the quality of >>>>> Cassandra 4.1 release. Should this person now invest in reviewing Accord >>>>> or not? It's impossible to know. Again, in hindsight we know that the >>>>> answer is no, but your suggestion again would require the person to >>>>> review all active feature branches just in case. >>>>> >>>>> >>>>> As for 2 and 3, I certainly observe an assumption that contributors have >>>>> expected to review after a rebase. But I don't see this as a significant >>>>> topic to argue about. If indeed the rebase is as easy as Benedict >>>>> advertised, then we should just do the rebase because apparently it can >>>>> be done faster than it took me to write this email :-) (But yes, >>>>> conversely, it seems then that the rebase is not a big reason to hold off >>>>> from reviewing either.) >>>>> >>>>> henrik >>>>> >>>>> >>>>> On Tue, Jan 24, 2023 at 9:29 PM Josh McKenzie <jmcken...@apache.org> >>>>> wrote: >>>>>> __ >>>>>> 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://urldefense.com/v3/__https://www.facebook.com/datastax__;!!PbtH5S7Ebw!ep4b-HH4HnBcGPT32sQbstaimEP5eIigJGvIpXgHKHxWq4uyqmNUiaz6DwjozGhRlQX9M2F7yZrdLA1y1UUJDw$> >>>>>>>>>>> __ <https://twitter.com/datastax> __ >>>>>>>>>>> <https://urldefense.com/v3/__https://www.linkedin.com/company/datastax/__;!!PbtH5S7Ebw!ep4b-HH4HnBcGPT32sQbstaimEP5eIigJGvIpXgHKHxWq4uyqmNUiaz6DwjozGhRlQX9M2F7yZrdLA2L93GKGQ$> >>>>>>>>>>> __ <https://github.com/datastax/> >>>>>> >>>>> >>>>> >>>>> -- >>>>> >>>>> >>>>> >>>>> *Henrik Ingo* >>>>> >>>>> *c*. +358 40 569 7354 >>>>> >>>>> *w*. _www.datastax.com_ >>>>> >>>>> __ >>>>> <https://urldefense.com/v3/__https://www.facebook.com/datastax__;!!PbtH5S7Ebw!aErnkmNEcHfMbh25akBV7vcAiYPa1n5NP_jsoWyV8MkZVbjAjXJeN0UjzgHhDjFXchva1Vu8u9DINuK_MhB-$> >>>>> __ <https://twitter.com/datastax> __ >>>>> <https://urldefense.com/v3/__https://www.linkedin.com/company/datastax/__;!!PbtH5S7Ebw!aErnkmNEcHfMbh25akBV7vcAiYPa1n5NP_jsoWyV8MkZVbjAjXJeN0UjzgHhDjFXchva1Vu8u9DINkBHA_Fu$> >>>>> __ <https://github.com/datastax/> >>>>> >>> >>> >>> -- >>> >>> >>> >>> *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/> >>> >> >> >> -- >> >> >> >> *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/>
