> 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 > <mailto: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 > <mailto: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 >> <mailto: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 >> <mailto: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 >>> <mailto: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 >>> <mailto: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 >>>> <mailto: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 >>>> <mailto: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 >>>> <mailto: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 >>>>> <mailto: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 <http://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 <http://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 <http://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 <http://www.datastax.com/> > <https://www.facebook.com/datastax> <https://twitter.com/datastax> > <https://www.linkedin.com/company/datastax/> <https://github.com/datastax/>
