Re: Merging CEP-15 to trunk

> It's been mentioned "always shippable trunk according to circleci". That's > not true: we are always shippable according to *either* CI. There are folk > just using ci-cassandra for our pre-commit gateway. It is important that you > don't trash the other CI system, particularly when it

Re: Merging CEP-15 to trunk

Hi Everyone, I hope you all had a lovely holiday period. > Thank you David and Caleb for your continuous work dealing with the practicalities involved in getting the merge to happen! And thank you Benedict for starting the thread – it is an important topic for us to continue as CEPs and feature

Re: Merging CEP-15 to trunk

Just an FYI, the Accord feature flag has landed in the cep-15-accord branch: https://github.com/apache/cassandra/commit/2e680a33c03ce66d4b1358e1a1cc11cf4ee0189f (btw, it implicitly fixes some of the dtests around the new Accord system keyspace, because Accord is now disabled by default.) On Tue,

Re: Merging CEP-15 to trunk

Thanks David, I think I am personally aligned with what you are saying. Thanks for clarifying your previous email. “ I do not believe anyone is arguing to merge Accord with bugs… bugs we know about are blockers for merge. There are pending improvements and features people are working on, but I

Re: Merging CEP-15 to trunk

> About this merge - I would personally love to see everything rebased and > things rerun in CI which no one has a doubt it will happen Yes, I think this is expected behavior that must happen before adding to trunk, or even just rebasing the feature branch; CI must be passing and flaky tests

Re: Merging CEP-15 to trunk

To clarify: my suggestion was not to block anything or block on green Jenkins. Running there was a suggestion for sanity check and because packaging and burn tests are run only in Jenkins. Also, to summarize, I had a few honest questions around things we expect on any ticket or which I am not

Re: Merging CEP-15 to trunk

> Don't we follow a principle of always shippable trunk? Also, I feel compelled to call out: we don't have perf regression testing we're doing publicly on the project, don't have code coverage analysis, and don't have long-running harry / nemeses-based soak testing to suss out subtle timing

Re: Merging CEP-15 to trunk

> Don't we follow a principle of always shippable trunk? This was actually a > reason why I sidelined the talk about post-merge review, because it implies > that the code wasn't "good enough"/perfect when it was first merged. We follow a principle of "always shippable trunk according to

Re: Merging CEP-15 to trunk

Do you mean as part of a blocking review, or just in general?I don’t honestly understand the focus on ninja fixes or rebasing, in either context. Why would a project rebase its work until ready to merge? Why would you worry that a team well resourced with experienced contributors (30+yrs between

Re: Merging CEP-15 to trunk

Benedict, Is it an issue to ask whether flaky tests will be addressed as per our project agreement? Or about ninja fixes and why a branch was not rebased during development? What did I miss? By the way I do not ask these questions to block anyone, even suggested to help with CI…it’s a pity this

Re: Merging CEP-15 to trunk

Review should primarily ask: "is this correct?" and "could this be done differently (clearer, faster, more correct, etc)?" Blocking reviews especially, because why else would a reasonable contributor want to block progress?These questions can of course be asked of implementation details for any

Re: Merging CEP-15 to trunk

Yes, reviews can and they happen any time and anyone can do them if they have time and interest before/after merge and anyone can raise questions or concerns. It was happening before, it happens now, it will happen in the future. The beauty of open source. About this merge - I would personally

Re: Merging CEP-15 to trunk

Yes, reviews can and they happen any time and anyone can do them if they have time and interest before/after merge and anyone can raise questions or concerns. It was happening before, it happens now, it will happen in the future. The beauty of open source. About this merge - I would personally

Re: Merging CEP-15 to trunk

> But during development, did you ever run nightly tests / all tests? I strongly recommend people I work with to use my script that modifies circle ci, which ops us into running all tests; I tend to see people use this as it makes CI faster, but has this by product as well. So yes, all tests

Re: Merging CEP-15 to trunk

On Tue, Jan 31, 2023 at 1:28 AM David Capwell wrote: > If the CI coverage isn't 100%, then we should just identify the gaps, and > focus on that while preparing to merge > > > It has 100% coverage that is done normally for trunk merges; which is a > pre-commit CI run in Circle OR Jenkins. > >

Re: Merging CEP-15 to trunk

> Does this mean there have also been nightly jenkins builds running? Is there > a history of such test results visible somewhere? If yes, I think that lends > a lot of credibility to the claim the process was as rigorous as it is for > trunk, and looking at the build history for a few minutes

Re: Merging CEP-15 to trunk

Ooops, I missed copy pasting this reply into my previous email: On Fri, Jan 27, 2023 at 11:21 PM Benedict wrote: > I'm realizing in retrospect this leaves ambiguity > > > Another misreading at least of the *intent* of these clauses, is that > they were to ensure that concerns about a *design

Re: Merging CEP-15 to trunk

Hi David On Fri, Jan 27, 2023 at 10:11 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 >

Re: Merging CEP-15 to trunk

I'm realizing in retrospect this leaves ambiguityAnother misreading at least of the intent of these clauses, is that they were to ensure that concerns about a design and approach are listened to, and addressed to the satisfaction of interested parties. It was essentially codifying the project’s

Re: Merging CEP-15 to trunk

> 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

Re: Merging CEP-15 to trunk

> 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

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

Hi Josh I chose to mainly reply to Benedict's latest email as a reply to both of you, but came back here only for a single higher level comment: I'm not aware of the project history of such mega reviews, other than years later, indirectly I have maybe felt the impact to quality that such large

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

contributors who didn't actively work on Accord, have assumed that they will be invited to review nowI 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

Re: Merging CEP-15 to trunk

I am +1 on what Jeremiah said Also, thanks Caleb for opening the tickets and the heads up on rebase On Tue, 24 Jan 2023 at 17:32, Jeremiah D Jordan wrote: > "hold the same bar for merges into a feature branch as trunk" > > > I think this is the key point here. If a feature branch is being

Re: Merging CEP-15 to trunk

So far in the in-progress rebase, the most annoying conflicts have been w/ my own trunk additions :p On Tue, Jan 24, 2023 at 4:32 PM Jeremiah D Jordan wrote: > "hold the same bar for merges into a feature branch as trunk" > > > I think this is the key point here. If a feature branch is being

Re: Merging CEP-15 to trunk

> "hold the same bar for merges into a feature branch as trunk" I think this is the key point here. If a feature branch is being treated as if it was a release branch with respect to commits that go into it then there should be no need to “do extra review pre merge to trunk”. The feature

Re: Merging CEP-15 to trunk

Cordial debate! <3 > - 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

Re: Merging CEP-15 to trunk

...and here they are: 1. CASSANDRA-18195 (Feature Flag for Accord Transactions) 1. CASSANDRA-18196 (Initial Merge of Accord Feature Branch to Trunk) 1. The

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

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 wrote: > I actually see people all the time making a final check before merge as > part of the review. And

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

On Tue, Jan 24, 2023 at 1:11 AM Jeff Jirsa 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)? > > There's no question that the feature branch already meets the minimum

Re: Merging CEP-15 to trunk

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

Re: Merging CEP-15 to trunk

> 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

Re: Merging CEP-15 to trunk

There is no merge-then-review. The work has been reviewed. This is identical to how reviews work as normal. If it helps your mental model, consider this a convenient atomic merge of many Jira that have independently met the standard project procedural requirements, as that is what it is.

Re: Merging CEP-15 to trunk

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?

Re: Merging CEP-15 to trunk

> > The sooner it’s in trunk, the more eyes it will draw, IMO, if you are > right about most contributors not having paid attention to a feature branch. > We all agree we want the feature branch incrementally merged sooner rather than later. IMHO any merge to trunk, and any rebase and squash of

Re: Merging CEP-15 to trunk

More eyes are of course always welcome. That said, there haven’t been many volunteers so far, despite its development going on for many months now, in the open, in official ASF repos. I suspect mainly because it’s pretty hard and not exactly very fun (speaking from experience). > If it

Re: Merging CEP-15 to trunk

What Benedict says is that the commits into cassandra/cep-15-accord and > cassandra-accord/trunk branch have all been vetted by at least two > committers already. Each authored by a Cassandra committer and then > reviewed by a Cassandra committer. That *is* our bar for merging into > Cassandra

Re: Merging CEP-15 to trunk

I might be completely off, but I think what others are referring to here is that 2 committers is the minimum bar, and for any commit there could be other contributors wishing to review some part or even in full what is being merged, and we would always allow for that, within reasonable time

Re: Merging CEP-15 to trunk

What Benedict says is that the commits into cassandra/cep-15-accord and cassandra-accord/trunk branch have all been vetted by at least two committers already. Each authored by a Cassandra committer and then reviewed by a Cassandra committer. That *is* our bar for merging into Cassandra trunk.

Re: Merging CEP-15 to trunk

These tickets have all met the standard integration requirements, so I’m > just unclear what “higher pre-commit gateway” you are referring to. > A merge into trunk deserves extra eyeballs than a merge into a feature branch. We can refer to this as a "higher pre-commit gateway" or a "second

Re: Merging CEP-15 to trunk

These tickets have all met the standard integration requirements, so I’m just unclear what “higher pre-commit gateway” you are referring to.I think the existing epics are probably more natural tickets to reference in the merge, eg 17091 and 17092.On 20 Jan 2023, at 11:04, Mick Semb Wever

Re: Merging CEP-15 to trunk

On Tue, 17 Jan 2023 at 10:29, Benedict wrote: > but the pre-commit gateway here is higher than the previous tickets being > worked on > > Which tickets, and why? > All tickets resolved in the feature branch to which you are now bringing from feature branch into trunk. A quick scan I see…

Re: Merging CEP-15 to trunk

> but the pre-commit gateway here is higher than the previous tickets being > worked on Which tickets, and why? > On 17 Jan 2023, at 07:43, Mick Semb Wever wrote: > >  > > >> Could you file a bug report with more detail about which classes you think >> are lacking adequate documentation

Re: Merging CEP-15 to trunk

Could you file a bug report with more detail about which classes you think > are lacking adequate documentation in each project, and what you would like > to see? > I suggest instead that we open a task ticket for the merge. I 100% agree with merging work incrementally, under a feature flag,

Re: Merging CEP-15 to trunk

Could you file a bug report with more detail about which classes you think are lacking adequate documentation in each project, and what you would like to see? We would welcome your participation.On 16 Jan 2023, at 17:28, Jacek Lewandowski wrote:Hi,It would be great if some documentation got

Re: Merging CEP-15 to trunk

Hi, It would be great if some documentation got added to the code you want to merge. To me, it would be enough to just quickly characterize on the class level what is the class for and what are the expectations. This is especially important for Accord API classes because now it is hard to review

Re: Merging CEP-15 to trunk

That’s fair, though for long term contributors probably the risk is relatively low on that front. I guess that’s something we can perhaps raise as part of each CEP if we envisage it taking several months of development?> Did we document this or is it in an email thread somewhere?It’s probably

Re: Merging CEP-15 to trunk

My only concern to merging (given all normal requirements are met) would be if there was a possibility that the feature would never be finished. Given all of the excitement and activity around accord, I do not think that is a concern here. So I see no reason not to merge incremental progress

Re: Merging CEP-15 to trunk

Did we document this or is it in an email thread somewhere? I don't see it on the confluence wiki nor does a cursory search of ponymail turn it up. What was it for something flagged experimental? 1. Same tests pass on the branch as to the root it's merging back to 2. 2 committers eyes on

Re: Merging CEP-15 to trunk

My goal isn’t to ask if others believe we have the right to merge, only to invite feedback if there are any specific concerns. Large pieces of work like this cause headaches and concerns for other contributors, and so it’s only polite to provide notice of our intention, since probably many

Re: Merging CEP-15 to trunk

I haven’t been following the progress of the feature branch, but I would think the requirements for merging it into master would be the same as any other merge. A subset of those requirements being: Is the code to be merged in releasable quality? Is it disabled by a feature flag by default if