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 <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 >> >> >> <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/>
