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