Re: Merging CEP-15 to trunk

[David Capwell]

> 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 comes to parity of the 
> tests that we run.  If you introduce new tests in one CI it is on you to 
> introduce it to the other CI, and therefore run both CI pre-merge.  This does 
> not imply a merge is blocked on green ci-cassandra.a.o

I had no intention to “trash” Jenkins, my reaction was to the statements that a 
lack of Jenkins means we are not following the process.  I am fully aware that 
there is a lot of work here, and know many of us (including myself) want to 
migrate away from Circle Ci.


> On Feb 1, 2023, at 3:24 PM, Mick Semb Wever  wrote:
> 
> 
> 
>> 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 branches become a thing!
> 
> Reading the thread I see some quite distinct perspectives, and it's making it 
> difficult to have as constructive a dialogue as we should wish for.  As is 
> usual we see this balanced by the  productive collaboration happening but 
> hidden in the jira tickets.
> 
> I want to raise the following points, not as things to solve necessarily but 
> to help us appreciate just how different our PoVs are and how easy it is for 
> us to be talking past each other. If you immediately disagree with any of 
> them I suggest you read below how my PoV gets me there.
> 
> 1. Work based on a notion of meritocracy. If you do the work, or have done 
> the work, your input weighs more.
> 2. Promote teamwork and value an inclusive culture.
> 3. Where the work is, or has been committed to, can be considered irrelevant 
> (in the context of this thread).
> 4. There are different types of reviews. We want to embrace them all.
> 5. We gate on either CI (not only circleci). Do not trash or reduce parity in 
> the other.
> 6. A feature branch != release branch/trunk. 
> 7. The cep-15-accord was not ready to merge, this thread helped smoke out a 
> number of minor things.
> 8. Merging of feature branches should be quick and light team-work.
> 
> 
> Going into these in more detail…
> 
> 1+3) 
> If a patch from a pmc engineer has been reviewed by two other pmc engineers,  
> CI has been run,  and they are about to merge,  a new reviewer interjecting 
> late can expect very little to happen pre-merge.  If there's something 
> serious it needs to be explained quickly.  We value people's time, 
> productiveness, and expertise.
> 
> This applies everywhere in the project: from tickets with patch files to CEP 
> work and feature branches. 
> 
> 2)
> The more eyes the better.  Everything we do is transparent, and we should 
> always be receptive and by default open to new input regardless of where in 
> the process we are and where it comes from.  First be receptive and hear 
> people out, afterwards make the "now, or later" decision.  Take the time to 
> explain why a "we'll do it later" decision – it might not be obvious to 
> everyone, it helps newcomers understand the project dynamics, and it sets 
> project precedence.  Use (1) as needed. 
> 
> 4)
> A lot of the thread has focused on reviews of Accord and its technical 
> components.  There are other types of reviews.  Examples are docs (internal 
> and external), the build system, our CIs and integrations, releases and ASF 
> policies.  Henrik pointed out that it is wasteful for these reviews to happen 
> early in the review window.  In addition, they are quite small in comparison 
> to initial technical reviews and equally quick to address. The larger the 
> patch the more we can expect others stepping in with these types of tail-end 
> reviews.
> 
> If we think of the review process as promoting teamwork and an inclusive 
> culture, we can also think of reviews akin to pair-programming that help 
> mentor those less knowledgeable in an area of the code.  This needs to be 
> time-boxed ofc, but IMHO we should keep an open mind to it – these types of 
> reviews do surprisingly lead to bug fixes and improvements with the reviewer 
> acting as a rubber-duck. This type of review we would want early, not late.
> 
> There's also another type of review here.  If the author of another CEP has 
> another feature branch about to merge, they may want to review the branch to 
> get ahead of possible incoming conflicts.  This is a review that cares 
> nothing about Accord itself.  Their evaluation is if there's any project-wide 
> ROI if some changes happen before the merge.  Again, teamwork.
> 
> 5)
> It's been mentioned "always shippable trunk according to circleci".  That's 
> not true: we are always sh

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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 branches become a thing!

Reading the thread I see some quite distinct perspectives, and it's making
it difficult to have as constructive a dialogue as we should wish for.  As
is usual we see this balanced by the  productive collaboration happening
but hidden in the jira tickets.

I want to raise the following points, not as things to solve necessarily
but to help us appreciate just how different our PoVs are and how easy it
is for us to be talking past each other. If you immediately disagree with
any of them I suggest you read below how my PoV gets me there.

1. Work based on a notion of meritocracy. If you do the work, or have done
the work, your input weighs more.
2. Promote teamwork and value an inclusive culture.
3. Where the work is, or has been committed to, can be considered
irrelevant (in the context of this thread).
4. There are different types of reviews. We want to embrace them all.
5. We gate on either CI (not only circleci). Do not trash or reduce parity
in the other.
6. A feature branch != release branch/trunk.
7. The cep-15-accord was not ready to merge, this thread helped smoke out a
number of minor things.
8. Merging of feature branches should be quick and light team-work.


Going into these in more detail…

1+3)
If a patch from a pmc engineer has been reviewed by two other pmc
engineers,  CI has been run,  and they are about to merge,  a new reviewer
interjecting late can expect very little to happen pre-merge.  If there's
something serious it needs to be explained quickly.  We value people's
time, productiveness, and expertise.

This applies everywhere in the project: from tickets with patch files to
CEP work and feature branches.

2)
The more eyes the better.  Everything we do is transparent, and we should
always be receptive and by default open to new input regardless of where in
the process we are and where it comes from.  First be receptive and hear
people out, afterwards make the "now, or later" decision.  Take the time to
explain why a "we'll do it later" decision – it might not be obvious to
everyone, it helps newcomers understand the project dynamics, and it sets
project precedence.  Use (1) as needed.

4)
A lot of the thread has focused on reviews of Accord and its technical
components.  There are other types of reviews.  Examples are docs (internal
and external), the build system, our CIs and integrations, releases and ASF
policies.  Henrik pointed out that it is wasteful for these reviews to
happen early in the review window.  In addition, they are quite small in
comparison to initial technical reviews and equally quick to address. The
larger the patch the more we can expect others stepping in with these types
of tail-end reviews.

If we think of the review process as promoting teamwork and an inclusive
culture, we can also think of reviews akin to pair-programming that help
mentor those less knowledgeable in an area of the code.  This needs to be
time-boxed ofc, but IMHO we should keep an open mind to it – these types of
reviews do surprisingly lead to bug fixes and improvements with the
reviewer acting as a rubber-duck. This type of review we would want early,
not late.

There's also another type of review here.  If the author of another CEP has
another feature branch about to merge, they may want to review the branch
to get ahead of possible incoming conflicts.  This is a review that cares
nothing about Accord itself.  Their evaluation is if there's any
project-wide ROI if some changes happen before the merge.  Again, teamwork.

5)
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 comes to parity
of the tests that we run.  If you introduce new tests in one CI it is on
you to introduce it to the other CI, and therefore run both CI pre-merge.
This does not imply a merge is blocked on green ci-cassandra.a.o

6)
It's been mentioned that code committed to a feature branch can be
considered finalised, i.e. the review window is closed.  I don't buy this.
We don't cut releases off feature branches and don't have a policy of
"always stable feature-branch".  Both (4) and (5) help illustrate this.
Thinking about the different types of reviews and concerns we have on trunk
was my background to presuming the review window is open until the final
atomic commits to our release branches.  And our stable trunk agreement and
efforts is my presumption that trunk is to be treated like a release
branch. This supports (3), it makes no difference if the work could be in a

Re: Merging CEP-15 to trunk

[Caleb Rackliffe]

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, Jan 31, 2023 at 3:08 PM Ekaterina Dimitrova 
wrote:

> 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 don’t believe I know of us trying to
> merge bugs into trunk.  If there are bugs found we should address before
> merge, but improvements can happen after trunk merge (depending on timing,
> before merge is fine).
>
> I do hope this comment doesn’t cause a rabbit hole of “what is a bug vs
> what is an improvement”….”
>
> No doubts on my end personally
>
> Excited to see Accord landing :-)
>
> On Tue, 31 Jan 2023 at 15:35, David Capwell  wrote:
>
>> 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 (as you called out) must be resolved.  Nothing is special from
>> a process point of view as this is already the expected process.  I know
>> Caleb is working on it and uses my circleci script, which leverages the
>> _repeat job to help flesh this out.  With the merge to trunk it “should”
>> detect all new tests and run them, so its fair to expect this to be stable.
>>
>> I am not sure I understand this -
>>
>>
>> Sorry, I was trying to be explicit on the current state of test failures,
>> but was not clear.
>>
>> Accord adds a new system table to make state durable (similar to the
>> paxos table).  This added table causes a well known subset of python dtests
>> to fail as they are checking for the set of keyspaces/tables and detecting
>> a new one added, causing the test to fail.  We have been ignoring these
>> rather than changing python-dtest that way we pull in latest changes there
>> rather than forking and freezing the branch we use.  We will 100% update
>> python dtest along with the merge to trunk similar to all other
>> cross-cutting work done.  Trunk must not fail due to this table getting
>> added and must 100% be dealt with at the same time we merge.
>>
>> Do you plan to merge before fixing the tests? I am confused
>>
>>
>> Yes, its a blocker for trunk merge.
>>
>> I think Henrik brought an interesting point about releasable trunk. I
>> don’t think that we are yet there with the Jenkins issues, but we are
>> getting close and we strive for that, no?
>>
>>
>> As Josh calls out, we require releasable trunk as defined by Circle CI
>> (change was made in 4.1); my understanding is that people are working to
>> improve Jenkins, and once this work is done we can migrate off Circle CI.
>> Until that happens we need to make sure Circle CI is stable for every
>> commit.
>>
>> 3) what is the story with the ninja fixes?
>>
>>
>> Can’t speak for 100% of them, but from the ones I have seen it boils down
>> to “our merge process relies on humans to do the right thing, and people
>> mess up some times”, which sadly is true in trunk.  To avoid calling anyone
>> out other than myself, lets look at a specific one that I did
>>
>> $ git show f8243f41c9e96c4a0390558086ece078b0b6b15c
>> commit f8243f41c9e96c4a0390558086ece078b0b6b15c
>> Author: David Capwell 
>> Date:   Mon Jan 9 13:20:58 2023 -0800
>>
>> Ninja: Add AccordTestUtils.parse which was missing in the latest
>> commit
>>
>> diff --git
>> a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
>> b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
>> index 4adad32d8a..20142c439b 100644
>> --- a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
>> +++ b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
>> @@ -174,6 +174,14 @@ public class AccordTestUtils
>>  return statement.createTxn(ClientState.forInternalCalls(),
>> options);
>>  }
>>
>> +public static TransactionStatement parse(String query)
>> +{
>> +TransactionStatement.Parsed parsed =
>> (TransactionStatement.Parsed) QueryProcessor.parseStatement(query);
>> +Assert.assertNotNull(parsed);
>> +TransactionStatement statement = (TransactionStatement)
>> parsed.prepare(ClientState.forInternalCalls());
>> +return statement;
>> +}
>> +
>>  public static Txn createTxn(int readKey, int... writeKeys)
>>  {
>>
>>  StringBuilder sb = new StringBuilder("BEGIN TRANSACTION\n”);
>>
>> I merged but a specific method was missing, this caused the branch to
>> fail to compile so I ninja pa

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 don’t believe I know of us trying to
merge bugs into trunk.  If there are bugs found we should address before
merge, but improvements can happen after trunk merge (depending on timing,
before merge is fine).

I do hope this comment doesn’t cause a rabbit hole of “what is a bug vs
what is an improvement”….”

No doubts on my end personally

Excited to see Accord landing :-)

On Tue, 31 Jan 2023 at 15:35, David Capwell  wrote:

> 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 (as you called out) must be resolved.  Nothing is special from
> a process point of view as this is already the expected process.  I know
> Caleb is working on it and uses my circleci script, which leverages the
> _repeat job to help flesh this out.  With the merge to trunk it “should”
> detect all new tests and run them, so its fair to expect this to be stable.
>
> I am not sure I understand this -
>
>
> Sorry, I was trying to be explicit on the current state of test failures,
> but was not clear.
>
> Accord adds a new system table to make state durable (similar to the paxos
> table).  This added table causes a well known subset of python dtests to
> fail as they are checking for the set of keyspaces/tables and detecting a
> new one added, causing the test to fail.  We have been ignoring these
> rather than changing python-dtest that way we pull in latest changes there
> rather than forking and freezing the branch we use.  We will 100% update
> python dtest along with the merge to trunk similar to all other
> cross-cutting work done.  Trunk must not fail due to this table getting
> added and must 100% be dealt with at the same time we merge.
>
> Do you plan to merge before fixing the tests? I am confused
>
>
> Yes, its a blocker for trunk merge.
>
> I think Henrik brought an interesting point about releasable trunk. I
> don’t think that we are yet there with the Jenkins issues, but we are
> getting close and we strive for that, no?
>
>
> As Josh calls out, we require releasable trunk as defined by Circle CI
> (change was made in 4.1); my understanding is that people are working to
> improve Jenkins, and once this work is done we can migrate off Circle CI.
> Until that happens we need to make sure Circle CI is stable for every
> commit.
>
> 3) what is the story with the ninja fixes?
>
>
> Can’t speak for 100% of them, but from the ones I have seen it boils down
> to “our merge process relies on humans to do the right thing, and people
> mess up some times”, which sadly is true in trunk.  To avoid calling anyone
> out other than myself, lets look at a specific one that I did
>
> $ git show f8243f41c9e96c4a0390558086ece078b0b6b15c
> commit f8243f41c9e96c4a0390558086ece078b0b6b15c
> Author: David Capwell 
> Date:   Mon Jan 9 13:20:58 2023 -0800
>
> Ninja: Add AccordTestUtils.parse which was missing in the latest commit
>
> diff --git
> a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
> b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
> index 4adad32d8a..20142c439b 100644
> --- a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
> +++ b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
> @@ -174,6 +174,14 @@ public class AccordTestUtils
>  return statement.createTxn(ClientState.forInternalCalls(),
> options);
>  }
>
> +public static TransactionStatement parse(String query)
> +{
> +TransactionStatement.Parsed parsed =
> (TransactionStatement.Parsed) QueryProcessor.parseStatement(query);
> +Assert.assertNotNull(parsed);
> +TransactionStatement statement = (TransactionStatement)
> parsed.prepare(ClientState.forInternalCalls());
> +return statement;
> +}
> +
>  public static Txn createTxn(int readKey, int... writeKeys)
>  {
>
>  StringBuilder sb = new StringBuilder("BEGIN TRANSACTION\n”);
>
> I merged but a specific method was missing, this caused the branch to fail
> to compile so I ninja patched by adding that method back; this was reviewed
> in slack with Benedict (if I remember correctly).
>
> I made a human judgement to merge via GH UI and bypass my merge scripts,
> as I had a Circle CI build showing success… I messed up and had to go back
> and fix it after commit.
>
> It is frustrating and I can imagine how after that much work around Accord
> people just want it in
>
>
> I do not believe anyone is arguing to merge Accord with bugs… bugs we know
> ab

Re: Merging CEP-15 to trunk

[David Capwell]

> 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 
(as you called out) must be resolved.  Nothing is special from a process point 
of view as this is already the expected process.  I know Caleb is working on it 
and uses my circleci script, which leverages the _repeat job to help flesh this 
out.  With the merge to trunk it “should” detect all new tests and run them, so 
its fair to expect this to be stable.

> I am not sure I understand this - 


Sorry, I was trying to be explicit on the current state of test failures, but 
was not clear.

Accord adds a new system table to make state durable (similar to the paxos 
table).  This added table causes a well known subset of python dtests to fail 
as they are checking for the set of keyspaces/tables and detecting a new one 
added, causing the test to fail.  We have been ignoring these rather than 
changing python-dtest that way we pull in latest changes there rather than 
forking and freezing the branch we use.  We will 100% update python dtest along 
with the merge to trunk similar to all other cross-cutting work done.  Trunk 
must not fail due to this table getting added and must 100% be dealt with at 
the same time we merge.

> Do you plan to merge before fixing the tests? I am confused


Yes, its a blocker for trunk merge.  

> I think Henrik brought an interesting point about releasable trunk. I don’t 
> think that we are yet there with the Jenkins issues, but we are getting close 
> and we strive for that, no?


As Josh calls out, we require releasable trunk as defined by Circle CI (change 
was made in 4.1); my understanding is that people are working to improve 
Jenkins, and once this work is done we can migrate off Circle CI.  Until that 
happens we need to make sure Circle CI is stable for every commit.

> 3) what is the story with the ninja fixes? 


Can’t speak for 100% of them, but from the ones I have seen it boils down to 
“our merge process relies on humans to do the right thing, and people mess up 
some times”, which sadly is true in trunk.  To avoid calling anyone out other 
than myself, lets look at a specific one that I did

$ git show f8243f41c9e96c4a0390558086ece078b0b6b15c
commit f8243f41c9e96c4a0390558086ece078b0b6b15c
Author: David Capwell 
Date:   Mon Jan 9 13:20:58 2023 -0800

Ninja: Add AccordTestUtils.parse which was missing in the latest commit

diff --git a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java 
b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
index 4adad32d8a..20142c439b 100644
--- a/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
+++ b/test/unit/org/apache/cassandra/service/accord/AccordTestUtils.java
@@ -174,6 +174,14 @@ public class AccordTestUtils
 return statement.createTxn(ClientState.forInternalCalls(), options);
 }

+public static TransactionStatement parse(String query)
+{
+TransactionStatement.Parsed parsed = (TransactionStatement.Parsed) 
QueryProcessor.parseStatement(query);
+Assert.assertNotNull(parsed);
+TransactionStatement statement = (TransactionStatement) 
parsed.prepare(ClientState.forInternalCalls());
+return statement;
+}
+
 public static Txn createTxn(int readKey, int... writeKeys)
 {
 StringBuilder sb = new StringBuilder("BEGIN TRANSACTION\n”);

I merged but a specific method was missing, this caused the branch to fail to 
compile so I ninja patched by adding that method back; this was reviewed in 
slack with Benedict (if I remember correctly).

I made a human judgement to merge via GH UI and bypass my merge scripts, as I 
had a Circle CI build showing success… I messed up and had to go back and fix 
it after commit.

> It is frustrating and I can imagine how after that much work around Accord 
> people just want it in


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 don’t believe I know of us trying to merge bugs 
into trunk.  If there are bugs found we should address before merge, but 
improvements can happen after trunk merge (depending on timing, before merge is 
fine).

I do hope this comment doesn’t cause a rabbit hole of “what is a bug vs what is 
an improvement”….


> On Jan 31, 2023, at 8:20 AM, Josh McKenzie  wrote:
> 
>> 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 circleci" as of 
> cutting 4.1, which has been adhered to in this case. Ther

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 clear about myself, even like from project
hygiene perspective. I am not worried or blocking anyone, neither
questioning the experience and knowledge of the engineers behind Accord. I
don’t believe I saw also anyone else questioning that too.

Agreed also that releasable trunk has a meaning that is not 100% covered or
even clear. Just from CI perspective that will mean green CI which as I
said earlier - we still don’t have in Jenkins due to numerous issues.

On Tue, 31 Jan 2023 at 11:25, Josh McKenzie  wrote:

> 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 issues at this time. So calling what we're doing on the ASF
> side "always shippable trunk" is leaving a lot of lift and toil up to folks
> working on these tags on their own infra that goes into each release.
>
> Which vexes me.
>
> On Tue, Jan 31, 2023, at 11:20 AM, Josh McKenzie wrote:
>
> 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 circleci" as
> of cutting 4.1, which has been adhered to in this case. There's every
> likelihood that ASF CI will detonate when this is merged in, but it's also
> already *currently* detonating repeatedly which we're working on so...
> /shrug
>
> If we block Accord merging on green ASF CI, we'll be in the same boat we
> were in with 4.1 and never ship until we tear certain things down to the
> studs and rebuild them. I don't think it's reasonable to put that burden
> (ASF CI must be green) on *any* ticket right now, much less one that has
> a potentially high integration cost to the entire project if it stagnates
> over time.
>
> plenty of experience with situations where even if an engineer, including
> myself sometimes, wanted to work on fixing some technical debt, the
> employer's project management processes simply wouldn't prioritize that
> work and it was left for years
>
> Seems like it's a healthy mix of debt and bikeshedding for us
> historically. The former we don't want to sneak in, the latter, well, that
> I'm less concerned about personally. :)
>
> And I think part of the "two committers +1" bar is about trying to keep
> our debt low.
>
> On Tue, Jan 31, 2023, at 2:45 AM, Benedict wrote:
>
>
> 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 them) don’t know how to work correctly with
> ninja fixes? These are all minor details, surely?
>
> I understand your concern around flaky tests, particularly since it seems
> other work has let some slip through. I don’t believe this is a blocking
> review concern, as it represents its own blocking requirement. I believe I
> have responded positively to this query already though?
>
>
> On 31 Jan 2023, at 01:46, Ekaterina Dimitrova 
> wrote:
>
> 
>
> 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 part was dismissed…
>
>  I can see that Caleb is handling the things around the merge ticket with
> high attention to the details as always. I can only thank him!
>
> At this point I see this thread mostly as - this is the first feature
> branch since quite some time, people are unsure about certain things -
> let’s see how we handle this for the next time to be more efficient and
> clear.  I think you already took some actions in your suggestion earlier
> today with the document around comments.
>
> On Mon, 30 Jan 2023 at 20:16, Benedict  wrote:
>
>
> 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
> CEP.
>
> I have said before, a proposal to conduct a blocking review of this kind -
> if very late in my view - would be valid, though ti

Re: Merging CEP-15 to trunk

[Josh McKenzie]

> 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 
issues at this time. So calling what we're doing on the ASF side "always 
shippable trunk" is leaving a lot of lift and toil up to folks working on these 
tags on their own infra that goes into each release.

Which vexes me.

On Tue, Jan 31, 2023, at 11:20 AM, Josh McKenzie wrote:
>> 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 circleci" as of 
> cutting 4.1, which has been adhered to in this case. There's every likelihood 
> that ASF CI will detonate when this is merged in, but it's also already 
> *currently* detonating repeatedly which we're working on so... /shrug
> 
> If we block Accord merging on green ASF CI, we'll be in the same boat we were 
> in with 4.1 and never ship until we tear certain things down to the studs and 
> rebuild them. I don't think it's reasonable to put that burden (ASF CI must 
> be green) on *any* ticket right now, much less one that has a potentially 
> high integration cost to the entire project if it stagnates over time.
> 
>> plenty of experience with situations where even if an engineer, including 
>> myself sometimes, wanted to work on fixing some technical debt, the 
>> employer's project management processes simply wouldn't prioritize that work 
>> and it was left for years
> Seems like it's a healthy mix of debt and bikeshedding for us historically. 
> The former we don't want to sneak in, the latter, well, that I'm less 
> concerned about personally. :)
> 
> And I think part of the "two committers +1" bar is about trying to keep our 
> debt low.
> 
> On Tue, Jan 31, 2023, at 2:45 AM, Benedict wrote:
>> 
>> 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 them) don’t know how to work correctly with ninja fixes? These are 
>> all minor details, surely?
>> 
>> I understand your concern around flaky tests, particularly since it seems 
>> other work has let some slip through. I don’t believe this is a blocking 
>> review concern, as it represents its own blocking requirement. I believe I 
>> have responded positively to this query already though?
>> 
>> 
>>> On 31 Jan 2023, at 01:46, Ekaterina Dimitrova  wrote:
>>> 
>>> 
>>> 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 part was dismissed… 
>>> 
>>>  I can see that Caleb is handling the things around the merge ticket with 
>>> high attention to the details as always. I can only thank him! 
>>> 
>>> At this point I see this thread mostly as - this is the first feature 
>>> branch since quite some time, people are unsure about certain things - 
>>> let’s see how we handle this for the next time to be more efficient and 
>>> clear.  I think you already took some actions in your suggestion earlier 
>>> today with the document around comments. 
>>> 
>>> On Mon, 30 Jan 2023 at 20:16, Benedict  wrote:
 
 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 
 CEP. 
 
 I have said before, a proposal to conduct a blocking review of this kind - 
 if very late in my view - would be valid, though timeline would have to be 
 debated.
 
 Reviewers with weaker aspirations have plenty of time available to them - 
 two weeks have already passed, and another couple will likely yet (there 
 isn't a rush). But it is novel to propose that such optional reviews be 
 treated as blocking.
 
 
> On 30 Jan 2023, at 23:04, Henrik Ingo  wrote:
> 
> 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

Re: Merging CEP-15 to trunk

[Josh McKenzie]

> 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 circleci" as of 
cutting 4.1, which has been adhered to in this case. There's every likelihood 
that ASF CI will detonate when this is merged in, but it's also already 
*currently* detonating repeatedly which we're working on so... /shrug

If we block Accord merging on green ASF CI, we'll be in the same boat we were 
in with 4.1 and never ship until we tear certain things down to the studs and 
rebuild them. I don't think it's reasonable to put that burden (ASF CI must be 
green) on *any* ticket right now, much less one that has a potentially high 
integration cost to the entire project if it stagnates over time.

> plenty of experience with situations where even if an engineer, including 
> myself sometimes, wanted to work on fixing some technical debt, the 
> employer's project management processes simply wouldn't prioritize that work 
> and it was left for years
Seems like it's a healthy mix of debt and bikeshedding for us historically. The 
former we don't want to sneak in, the latter, well, that I'm less concerned 
about personally. :)

And I think part of the "two committers +1" bar is about trying to keep our 
debt low.

On Tue, Jan 31, 2023, at 2:45 AM, Benedict wrote:
> 
> 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 them) don’t know how to work correctly with ninja fixes? These are 
> all minor details, surely?
> 
> I understand your concern around flaky tests, particularly since it seems 
> other work has let some slip through. I don’t believe this is a blocking 
> review concern, as it represents its own blocking requirement. I believe I 
> have responded positively to this query already though?
> 
> 
>> On 31 Jan 2023, at 01:46, Ekaterina Dimitrova  wrote:
>> 
>> 
>> 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 part was dismissed… 
>> 
>>  I can see that Caleb is handling the things around the merge ticket with 
>> high attention to the details as always. I can only thank him! 
>> 
>> At this point I see this thread mostly as - this is the first feature branch 
>> since quite some time, people are unsure about certain things - let’s see 
>> how we handle this for the next time to be more efficient and clear.  I 
>> think you already took some actions in your suggestion earlier today with 
>> the document around comments. 
>> 
>> On Mon, 30 Jan 2023 at 20:16, Benedict  wrote:
>>> 
>>> 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 
>>> CEP. 
>>> 
>>> I have said before, a proposal to conduct a blocking review of this kind - 
>>> if very late in my view - would be valid, though timeline would have to be 
>>> debated.
>>> 
>>> Reviewers with weaker aspirations have plenty of time available to them - 
>>> two weeks have already passed, and another couple will likely yet (there 
>>> isn't a rush). But it is novel to propose that such optional reviews be 
>>> treated as blocking.
>>> 
>>> 
 On 30 Jan 2023, at 23:04, Henrik Ingo  wrote:
 
 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 and approach* are 
> listened to, and addressed to the satisfaction of interested parties. It 
> was essentially codifying the project’s long term etiquette around pieces 
> of work with either competing proposals or fundamental concerns. It has 
> historically helped to avoid escalation to vetoes, or reverting code 
> after commit. 
> 
> It wasn’t intended that *any* reason might be invoked, as seems to have 
> been inferred, and perhaps this should be clarified, though I had hoped 
> it would be captured by the word “reasonable". Minor concerns that 
> haven’t been caught by the initial review process can a

Re: Merging CEP-15 to trunk

[Benedict]

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 them) don’t know how to work correctly with ninja fixes? These are all minor details, surely?I understand your concern around flaky tests, particularly since it seems other work has let some slip through. I don’t believe this is a blocking review concern, as it represents its own blocking requirement. I believe I have responded positively to this query already though?On 31 Jan 2023, at 01:46, Ekaterina Dimitrova  wrote: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 part was dismissed…  I can see that Caleb is handling the things around the merge ticket with high attention to the details as always. I can only thank him! At this point I see this thread mostly as - this is the first feature branch since quite some time, people are unsure about certain things - let’s see how we handle this for the next time to be more efficient and clear.  I think you already took some actions in your suggestion earlier today with the document around comments. On Mon, 30 Jan 2023 at 20:16, Benedict  wrote: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 CEP. I have said before, a proposal to conduct a blocking review of this kind - if very late in my view - would be valid, though timeline would have to be debated.Reviewers with weaker aspirations have plenty of time available to them - two weeks have already passed, and another couple will likely yet (there isn't a rush). But it is novel to propose that such optional reviews be treated as blocking.On 30 Jan 2023, at 23:04, Henrik Ingo  wrote: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 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 long term etiquette around pieces of work with either competing proposals or fundamental concerns. It has historically helped to avoid escalation to vetoes, or reverting code after commit. It wasn’t intended that any reason might be invoked, as seems to have been inferred, and perhaps this should be clarified, though I had hoped it would be captured by the word “reasonable". Minor concerns that haven’t been caught by the initial review process can always be addressed in follow-up work, as is very common.Wouldn't you expect such concerns to at least partially now have been covered in  the CEP discussion, up front? I would expect at most at this stage someone could validate that the implementation follows the CEP. But I wouldn't expect a debate on competing approaches at this stage.henrik-- Henrik Ingoc. +358 40 569 7354 w. www.datastax.com      

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 part was dismissed…

 I can see that Caleb is handling the things around the merge ticket with
high attention to the details as always. I can only thank him!

At this point I see this thread mostly as - this is the first feature
branch since quite some time, people are unsure about certain things -
let’s see how we handle this for the next time to be more efficient and
clear.  I think you already took some actions in your suggestion earlier
today with the document around comments.

On Mon, 30 Jan 2023 at 20:16, Benedict  wrote:

> 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
> CEP.
>
> I have said before, a proposal to conduct a blocking review of this kind -
> if very late in my view - would be valid, though timeline would have to be
> debated.
>
> Reviewers with weaker aspirations have plenty of time available to them -
> two weeks have already passed, and another couple will likely yet (there
> isn't a rush). But it is novel to propose that such optional reviews be
> treated as blocking.
>
> On 30 Jan 2023, at 23:04, Henrik Ingo  wrote:
>
> 
>
> 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 and approach* are
>> listened to, and addressed to the satisfaction of interested parties. It
>> was essentially codifying the project’s long term etiquette around pieces
>> of work with either competing proposals or fundamental concerns. It has
>> historically helped to avoid escalation to vetoes, or reverting code after
>> commit.
>>
>> It wasn’t intended that *any* reason might be invoked, as seems to have
>> been inferred, and perhaps this should be clarified, though I had hoped it
>> would be captured by the word “reasonable". Minor concerns that haven’t
>> been caught by the initial review process can always be addressed in
>> follow-up work, as is very common.
>>
>>
> Wouldn't you expect such concerns to at least partially now have been
> covered in  the CEP discussion, up front? I would expect at most at this
> stage someone could validate that the implementation follows the CEP. But I
> wouldn't expect a debate on competing approaches at this stage.
>
> henrik
> --
>
> Henrik Ingo
>
> c. +358 40 569 7354
>
> w. www.datastax.com
>
>   
> 
> 
>
>

Re: Merging CEP-15 to trunk

[Benedict]

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 CEP. I have said before, a proposal to conduct a blocking review of this kind - if very late in my view - would be valid, though timeline would have to be debated.Reviewers with weaker aspirations have plenty of time available to them - two weeks have already passed, and another couple will likely yet (there isn't a rush). But it is novel to propose that such optional reviews be treated as blocking.On 30 Jan 2023, at 23:04, Henrik Ingo  wrote: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 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 long term etiquette around pieces of work with either competing proposals or fundamental concerns. It has historically helped to avoid escalation to vetoes, or reverting code after commit. It wasn’t intended that any reason might be invoked, as seems to have been inferred, and perhaps this should be clarified, though I had hoped it would be captured by the word “reasonable". Minor concerns that haven’t been caught by the initial review process can always be addressed in follow-up work, as is very common.Wouldn't you expect such concerns to at least partially now have been covered in  the CEP discussion, up front? I would expect at most at this stage someone could validate that the implementation follows the CEP. But I wouldn't expect a debate on competing approaches at this stage.henrik-- Henrik Ingoc. +358 40 569 7354 w. www.datastax.com      

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 love to see everything rebased and
things rerun in CI which no one has a doubt it will happen. Benedict
already mentioned also the repeatable jobs will be used, thanks! (I know
how disappointing it can be to find a flaky test but it is way easier to
fix it while you are still on top of things, instead of, half year away for
example.)
I am not sure I understand this -
“ There exists test failures, but those are due to python-dtest not knowing
about the accord table breaking a few tests (blocker for trunk merge, it
means trunk dtest must now known about accord), or flaky tests that
committers tend to merge anyways (such as specific tests that fail often,
known issues, OS allocating of bind address, etc.)”
Do you plan to merge before fixing the tests? I am confused

I think Henrik brought an interesting point about releasable trunk. I don’t
think that we are yet there with the Jenkins issues, but we are getting
close and we strive for that, no?
My understanding at this point is:
1) we rebase and run CI, get final reviewer approval before a commit. I am
not saying it hasn’t happened with the Accord tickets,no, but I am still
not sure why the branch was not rebased to be honest. Probably there was
some reasoning I am just not familiar with it but I am definitely curious.
I find a lot of sense in agreeing in the future to keep feature branches
rebased when commits land there.
2) If we encounter new issues/flaky tests/review comments we unfortunately
get back to them and we fix them pre-commit on tickets. On the bright side,
again, it is way easier to fix them before things got in the mix and time
pass. And I believe this is the trunk agreement and the quality bar we try
to keep.
Also, knowing how you were always adhering strictly to two committers and
CI runs pre-commit to the feature branch plus Benedict’s explanation around
mainly additions of code, I don’t see any reason for anyone to be worried.
I am optimistic we will see very soon things landing in a good shape.

3) what is the story with the ninja fixes? Are they trivial ninja fixes as
Josh mentioned or additional small commits? Were they reviewed? Are they
going to be squashed? (Take these as honest questions, I just don’t know
the plan and I am trying to create my mental model). I will be happy to add
a section around ninja commits on our “how to commit page”

Unfortunately, I disagree that finding a bug long after something was
committed is the same as seeing one before commit. I see people on numerous
occasions fixing things and revising. It is frustrating and I can imagine
how after that much work around Accord people just want it in. But don’t
you want to close the lose ends earlier than later? Yes, you’d say this is
long-term committed but I am afraid if you are rebasing and squashing, I
still look at the situation in a bit different way. Just my experience with
Cassandra where constantly the most unexpected things fail me :-)

Interesting points around CI, I am wondering whether we want to run the
code in both systems before commit as Jenkins also runs packaging and burn
tests. I will be happy to help to push the branch into Jenkins or CircleCI
if help is needed. I don’t have time for a review but I don’t mind to help
revise the CI results and identify what is old and (hopefully nothing) what
is new.

Also, when the time approaches, shall we agree on a particular day and ask
people not to commit anything for a day or so maybe until you test and
commit? To give you the chance to wrap up without interruptions? I think it
will be nice to schedule something.

BR,
Ekaterina

On Mon, 30 Jan 2023 at 18:54, David Capwell  wrote:

> 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 should be run before merge.  There are examples of
> Jenkins only tests that are not run, but again this is due to existing
> limitations with Jenkins.
>
>
> On Jan 30, 2023, at 3:33 PM, Henrik Ingo  wrote:
>
> 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.
>>
>>
> Sure. And thanks.
>
> But during development, did you ever run nightly tests / all tests? I
> wouldn't want the night after merging to trunk to be the first time those
> are run.
>
> henrik
> --
>
> Henrik Ingo
>
> c. +358 40 569

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 love to see everything rebased and
things rerun in CI which no one has a doubt it will happen. Benedict
already mentioned also the repeatable jobs will be used, thanks! (I know
how disappointing it can be to find a flaky test but it is way easier to
fix it while you are still on top of things, instead of, half year away for
example.)
I am not sure I understand this -
“ There exists test failures, but those are due to python-dtest not knowing
about the accord table breaking a few tests (blocker for trunk merge, it
means trunk dtest must now known about accord), or flaky tests that
committers tend to merge anyways (such as specific tests that fail often,
known issues, OS allocating of bind address, etc.)”
Do you plan to merge before fixing the tests? I am confused

I think Henrik brought an interesting point about releasable trunk. I don’t
think that we are yet there with the Jenkins issues, but we are getting
close and we strive for that, no?
My understanding at this point is:
1) we rebase and run CI, get final reviewer approval before a commit. I am
not saying it hasn’t happened with the Accord tickets,no, but I am still
not sure why the branch was not rebased to be honest. Probably there was
some reasoning I am just not familiar with it but I am definitely curious.
I find a lot of sense in agreeing in the future to keep feature branches
rebased when commits land there.
2) If we encounter new issues/flaky tests/review comments we unfortunately
get back to them and we fix them pre-commit on tickets. On the bright side,
again, it is way easier to fix them before things got in the mix and time
pass. And I believe this is the trunk agreement and the quality bar we try
to keep.
Also, knowing how you were always adhering strictly to two committers and
CI runs pre-commit to the feature branch plus Benedict’s explanation around
mainly additions of code, I don’t see any reason for anyone to be worried.
I am optimistic we will see very soon things landing in a good shape.

3) what is the story with the ninja fixes? Are they trivial ninja fixes as
Josh mentioned or additional small commits? Were they reviewed? Are they
going to be squashed? (Take these as honest questions, I just don’t know
the plan and I am trying to create my mental model). I will be happy to add
a section around ninja commits on our “how to commit page”

Unfortunately, I disagree that finding a bug long after something was
committed is the same as seeing one before commit. I see people on numerous
occasions fixing things and revising. It is frustrating and I can imagine
how after that much work around Accord people just want it in. But don’t
you want to close the lose ends earlier than later? Yes, you’d say this is
long-term committed but I am afraid if you are rebasing and squashing, I
still look at the situation in a bit different way. Just my experience with
Cassandra where constantly the most unexpected things fail me :-)

Interesting points around CI, I am wondering whether we want to run the
code in both systems before commit as Jenkins also runs packaging and burn
tests. I will be happy to help to push the branch into Jenkins or CircleCI
if help is needed. I don’t have time for a review but I don’t mind to help
revise the CI results and identify what is old and (hopefully nothing) what
is new.

Also, when the time approaches, shall we agree on a particular day and ask
people not to commit anything for a day or so maybe until you test and
commit? To give you the chance to wrap up without interruptions? I think it
will be nice to schedule something.

BR,
Ekaterina


On Mon, 30 Jan 2023 at 18:33, Henrik Ingo  wrote:

> 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.
>>
>>
> Sure. And thanks.
>
> But during development, did you ever run nightly tests / all tests? I
> wouldn't want the night after merging to trunk to be the first time those
> are run.
>
> henrik
> --
>
> Henrik Ingo
>
> c. +358 40 569 7354
>
> w. www.datastax.com
>
>   
> 
> 
>
>

Re: Merging CEP-15 to trunk

[David Capwell]

> 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 should be run before merge.  There are examples of Jenkins 
only tests that are not run, but again this is due to existing limitations with 
Jenkins.

> On Jan 30, 2023, at 3:33 PM, Henrik Ingo  wrote:
> 
> 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.
> 
> 
> Sure. And thanks.
> 
> But during development, did you ever run nightly tests / all tests? I 
> wouldn't want the night after merging to trunk to be the first time those are 
> run.
> 
> henrik
> -- 
> 
> Henrik Ingo
> c. +358 40 569 7354 
> w. www.datastax.com 
>        
>    

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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.
>
>
Sure. And thanks.

But during development, did you ever run nightly tests / all tests? I
wouldn't want the night after merging to trunk to be the first time those
are run.

henrik
-- 

Henrik Ingo

c. +358 40 569 7354

w. www.datastax.com

  
  

Re: Merging CEP-15 to trunk

[David Capwell]

> 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 should put our 
> minds at ease. I can't see anything Accord related in Jenkins or Butler? But 
> perhaps there's a CircleCI dashboard somewhere?

The pre-commit process has been using Circle CI, this is due to limitations 
that have been impacting Jenkins (stability, resources, etc.).  Example JIRA 
https://issues.apache.org/jira/browse/CASSANDRA-18154 
 shows the CI results 
https://app.circleci.com/pipelines/github/dcapwell/cassandra/1793/workflows/0d9ffabd-8046-41e4-a19d-aee8da98ac00
 
.

There exists test failures, but those are due to python-dtest not knowing about 
the accord table breaking a few tests (blocker for trunk merge, it means trunk 
dtest must now known about accord), or flaky tests that committers tend to 
merge anyways (such as specific tests that fail often, known issues, OS 
allocating of bind address, etc.)

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

> On Jan 30, 2023, at 3:03 PM, Henrik Ingo  wrote:
> 
> 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 and approach are listened to, and 
> addressed to the satisfaction of interested parties. It was essentially 
> codifying the project’s long term etiquette around pieces of work with either 
> competing proposals or fundamental concerns. It has historically helped to 
> avoid escalation to vetoes, or reverting code after commit. 
> 
> It wasn’t intended that any reason might be invoked, as seems to have been 
> inferred, and perhaps this should be clarified, though I had hoped it would 
> be captured by the word “reasonable". Minor concerns that haven’t been caught 
> by the initial review process can always be addressed in follow-up work, as 
> is very common.
> 
> 
> Wouldn't you expect such concerns to at least partially now have been covered 
> in  the CEP discussion, up front? I would expect at most at this stage 
> someone could validate that the implementation follows the CEP. But I 
> wouldn't expect a debate on competing approaches at this stage.
> 
> henrik
> -- 
> 
> Henrik Ingo
> c. +358 40 569 7354 
> w. www.datastax.com 
>        
>    

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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 and approach* are
> listened to, and addressed to the satisfaction of interested parties. It
> was essentially codifying the project’s long term etiquette around pieces
> of work with either competing proposals or fundamental concerns. It has
> historically helped to avoid escalation to vetoes, or reverting code after
> commit.
>
> It wasn’t intended that *any* reason might be invoked, as seems to have
> been inferred, and perhaps this should be clarified, though I had hoped it
> would be captured by the word “reasonable". Minor concerns that haven’t
> been caught by the initial review process can always be addressed in
> follow-up work, as is very common.
>
>
Wouldn't you expect such concerns to at least partially now have been
covered in  the CEP discussion, up front? I would expect at most at this
stage someone could validate that the implementation follows the CEP. But I
wouldn't expect a debate on competing approaches at this stage.

henrik
-- 

Henrik Ingo

c. +358 40 569 7354

w. www.datastax.com

  
  

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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
> 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 know the Accord team has all the time been open to and even invited
participation from other contributors. I believe what happened was simply
that some of us thought "the process is Y" and then were surprised to be
rejected when trying to do Y. I think this thread has already surfaced the
different assumptions and what might be the most meaningful consensus for
future similar projects.



> 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.
>
>
Yup.




> 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.
>
>
I believe Ekaterina was the first one to talk to me about the possibility
of reviewing post merge. I have to admit I have simply not taking it
seriously at the time. My background is perhaps in more corporate open
source development, and I have plenty of experience with situations where
even if an engineer, including myself sometimes, wanted to work on fixing
some technical debt, the employer's project management processes simply
wouldn't prioritize that work and it was left for years. Now that several
people have assured me that this is actually a thing in the Cassandra
project, I will try to embrace that option. It's certainly a nice tool to
not hold up the merge more than necessary.


On Fri, Jan 27, 2023 at 10:22 PM Josh McKenzie  wrote:

> 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.
>
>
Ah, this is a great perspective! I can immediately think of follow-up
thoughts...

1. 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. (But life is of course never that black and white.) Anyway... With
always shippable trunk, in theory it shouldn't make a difference whether
this is happening a week before GA or months before.

But, more importantly...

2. You and others are saying the Accord feature branch followed the same
principles as development on 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 should put our minds at ease. I can't see
anything Accord related in Jenkins or Butler? But perhaps there's a
Ci

Re: Merging CEP-15 to trunk

[Benedict]

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 long term etiquette around pieces of work with either competing proposals or fundamental concerns. It has historically helped to avoid escalation to vetoes, or reverting code after commit. It wasn’t intended that any reason might be invoked, as seems to have been inferred, and perhaps this should be clarified, though I had hoped it would be captured by the word “reasonable". Minor concerns that haven’t been caught by the initial review process can always be addressed in follow-up work, as is very common.On 27 Jan 2023, at 20:22, Josh McKenzie  wrote: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.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  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 Gove

Re: Merging CEP-15 to trunk

[Josh McKenzie]

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

Re: Merging CEP-15 to trunk

[David Capwell]

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

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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 
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  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  wrote:
>>
>> 
>> Thanks Josh
>>
>> Since you mentioned the CEP process, I should also mention one sentiment
>> you omitted, but worth stating explicitly:
>

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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
commits (late in the release cycle...) typically have on a complex code
base. But sure, I can see how that could have happened and understand it
would then impact this discussion too.

Fundamentally we have two conflicting interests at play:
 - merging smaller incremental changes is preferable to large merges
 - merging incomplete work is usually a bad idea, it's better to work on a
feature branch until some kind of mvp or v1 level of functionality is met.

We'll just have to learn to balance both of these.

Related to the history though, one thing that has changed is the
introduction of CEPs. I actually expect this to make a huge difference
compared to historical traumatic experiences. Because at this point we
definitely do not need to discuss a) whether we want transactions in the
first place, or b) how they should (have) be(en)  implemented.

Also, the fact that code going into the feature branch was already reviewed
with the same rigor as a trunk merge would, should of course make a big
difference too. So in summary, I'm optimistic that the processes we are
following today will work better than what was in the past. (If anything,
the CI worries me a lot more than the review process. If we had an
automated merge pipeline that would do the rebase-test-merge in an
automated and uncompromising way, I bet even this discussion would have
been more relaxed.)

henrik

On Wed, Jan 25, 2023 at 12:11 AM Josh McKenzie  wrote:

> 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 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.
>
> If this was a miscommunication on this instance rectifying it will of
> course require compromise from all parties. Good learning for future
> engagement and hopefully the outcome of this discussion is clearer norms as
> a project so we don't end up with this miscommunication in the future.
>
> 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
>
> I think this is something we can either choose to make a formal
> requirement for feature branches in ASF git (all code that goes in has 2
> committers hands on) or not. If folks want to work on other feature
> branches in other forks w/out this bar and then have a "mega review" at the
> end, I suppose that's their prerogative. Many of us that have been on the
> project for years have _significant emotional and psychological scars_ from
> that approach however, and multiple large efforts have failed at the
> "mega-review and merge" step. So I wouldn't advocate for that approach (and
> it's the only logical alternative I can think of to incremental bar of
> quality reinforcement throughout a work cycle on a large feature over time).
>
> 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
>
> Right now we culturally tend to avoid merging code that doesn't do
> anything, for better or worse. We don't have a strong culture of either
> incremental merge in during development or of using the experimental flag
> for new features. Much of the tightly coupled nature of our codebase makes
> this a necessity for keeping velocity while working unfortunately. So in
> this case I would qualify that "it's not ready to be merged yet given our
> assumption that all code in the codebase should serve an active immediate
> purpose, not due to a lack of merge-level quality".
>
> The approach of "hold the same bar for merges into a feature branch as
> trunk" seems to be a compromise between Big Bang single commit drops and
> peppering trunk with a lot of "as yet dormant" incremental code as a large
> feature is built out. Not saying it's better or worse, just describing the
> contour of the tradeoffs as I see them.
>
> - 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.
>
> This is an excellent point. The only mitigation I'd see for this would be
> an additional review period or burden collectively before merge of a
> feature branch into trunk once something has crossed a threshold of success
> as to be included, or stepping aw

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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

Re: Merging CEP-15 to trunk

[Benedict]

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 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  wrote:Thanks JoshSince 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.)henrikOn Tue, Jan 24, 2023 at 9:29 PM Josh McKenzie 

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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 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 branch should follow what we do for all other things post review
> and pre merge to trunk.
>
> 1. Rebase the code
> a. If the rebase meant changing a bunch of stuff, ask a reviewer to look
> that over, then continue
> b. If the rebase didn’t change anything substantial continue.
> 2. Run CI on rebased code.
> 3. Push the code to trunk.
>
> -Jeremiah
>
>
> On Jan 24, 2023, at 4:10 PM, Josh McKenzie  wrote:
>
> 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 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.
>
> If this was a miscommunication on this instance rectifying it will of
> course require compromise from all parties. Good learning for future
> engagement and hopefully the outcome of this discussion is clearer norms as
> a project so we don't end up with this miscommunication in the future.
>
> 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
>
> I think this is something we can either choose to make a formal
> requirement for feature branches in ASF git (all code that goes in has 2
> committers hands on) or not. If folks want to work on other feature
> branches in other forks w/out this bar and then have a "mega review" at the
> end, I suppose that's their prerogative. Many of us that have been on the
> project for years have _significant emotional and psychological scars_ from
> that approach however, and multiple large efforts have failed at the
> "mega-review and merge" step. So I wouldn't advocate for that approach (and
> it's the only logical alternative I can think of to incremental bar of
> quality reinforcement throughout a work cycle on a large feature over time).
>
> 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
>
> Right now we culturally tend to avoid merging code that doesn't do
> anything, for better or worse. We don't have a strong culture of either
> incremental merge in during development or of using the experimental flag
> for new features. Much of the tightly coupled nature of our codebase makes
> this a necessity for keeping velocity while working unfortunately. So in
> this case I would qualify that "it's not ready to be merged yet given our
> assumption that all code in the codebase should serve an active immediate
> purpose, not due to a lack of merge-level quality".
>
> The approach of "hold the same bar for merges into a feature branch as
> trunk" seems to be a compromise between Big Bang single commit drops and
> peppering trunk with a lot of "as yet dormant" incremental code as a large
> feature is built out. Not saying it's better or worse, just describing the
> contour of the tradeoffs as I see them.
>
> - 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.
>
> This is an excellent point. The only mitigation I'd see for this would be
> an additional review period or burden collectively before merge of a
> feature branch into trunk once something has crossed a threshold of success
> as to be included, or stepping away from a project where you don't have the
> cycles to stay up to date and review and trust that the other committers
> working on the project are making choices that are palatable and acceptable
> to you.
>
> If all API decisions hit the dev ML and the architecture conforms
> generally to the specification of the CEP, it seems to me that stepping
> back and trusting your fellow committers to Do The Right Thing is the
> optimal (and scalable) approach here?
>
> 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.
>
> I'd argue that there's 3 times to really invest in the quality o

Re: Merging CEP-15 to trunk

[Caleb Rackliffe]

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 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 branch should follow what we do for all other things post review
> and pre merge to trunk.
>
> 1. Rebase the code
> a. If the rebase meant changing a bunch of stuff, ask a reviewer to look
> that over, then continue
> b. If the rebase didn’t change anything substantial continue.
> 2. Run CI on rebased code.
> 3. Push the code to trunk.
>
> -Jeremiah
>
>
> On Jan 24, 2023, at 4:10 PM, Josh McKenzie  wrote:
>
> 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 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.
>
> If this was a miscommunication on this instance rectifying it will of
> course require compromise from all parties. Good learning for future
> engagement and hopefully the outcome of this discussion is clearer norms as
> a project so we don't end up with this miscommunication in the future.
>
> 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
>
> I think this is something we can either choose to make a formal
> requirement for feature branches in ASF git (all code that goes in has 2
> committers hands on) or not. If folks want to work on other feature
> branches in other forks w/out this bar and then have a "mega review" at the
> end, I suppose that's their prerogative. Many of us that have been on the
> project for years have _significant emotional and psychological scars_ from
> that approach however, and multiple large efforts have failed at the
> "mega-review and merge" step. So I wouldn't advocate for that approach (and
> it's the only logical alternative I can think of to incremental bar of
> quality reinforcement throughout a work cycle on a large feature over time).
>
> 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
>
> Right now we culturally tend to avoid merging code that doesn't do
> anything, for better or worse. We don't have a strong culture of either
> incremental merge in during development or of using the experimental flag
> for new features. Much of the tightly coupled nature of our codebase makes
> this a necessity for keeping velocity while working unfortunately. So in
> this case I would qualify that "it's not ready to be merged yet given our
> assumption that all code in the codebase should serve an active immediate
> purpose, not due to a lack of merge-level quality".
>
> The approach of "hold the same bar for merges into a feature branch as
> trunk" seems to be a compromise between Big Bang single commit drops and
> peppering trunk with a lot of "as yet dormant" incremental code as a large
> feature is built out. Not saying it's better or worse, just describing the
> contour of the tradeoffs as I see them.
>
> - 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.
>
> This is an excellent point. The only mitigation I'd see for this would be
> an additional review period or burden collectively before merge of a
> feature branch into trunk once something has crossed a threshold of success
> as to be included, or stepping away from a project where you don't have the
> cycles to stay up to date and review and trust that the other committers
> working on the project are making choices that are palatable and acceptable
> to you.
>
> If all API decisions hit the dev ML and the architecture conforms
> generally to the specification of the CEP, it seems to me that stepping
> back and trusting your fellow committers to Do The Right Thing is the
> optimal (and scalable) approach here?
>
> 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.
>
> I'd argue that there's 3 times to really invest in the quality

Re: Merging CEP-15 to trunk

[Jeremiah D Jordan]

> "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 branch 
should follow what we do for all other things post review and pre merge to 
trunk.

1. Rebase the code
a. If the rebase meant changing a bunch of stuff, ask a reviewer to 
look that over, then continue
b. If the rebase didn’t change anything substantial continue.
2. Run CI on rebased code.
3. Push the code to trunk.

-Jeremiah


> On Jan 24, 2023, at 4:10 PM, Josh McKenzie  wrote:
> 
> 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 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.
> If this was a miscommunication on this instance rectifying it will of course 
> require compromise from all parties. Good learning for future engagement and 
> hopefully the outcome of this discussion is clearer norms as a project so we 
> don't end up with this miscommunication in the future.
> 
>> 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
> I think this is something we can either choose to make a formal requirement 
> for feature branches in ASF git (all code that goes in has 2 committers hands 
> on) or not. If folks want to work on other feature branches in other forks 
> w/out this bar and then have a "mega review" at the end, I suppose that's 
> their prerogative. Many of us that have been on the project for years have 
> _significant emotional and psychological scars_ from that approach however, 
> and multiple large efforts have failed at the "mega-review and merge" step. 
> So I wouldn't advocate for that approach (and it's the only logical 
> alternative I can think of to incremental bar of quality reinforcement 
> throughout a work cycle on a large feature over time).
> 
>> 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
> Right now we culturally tend to avoid merging code that doesn't do anything, 
> for better or worse. We don't have a strong culture of either incremental 
> merge in during development or of using the experimental flag for new 
> features. Much of the tightly coupled nature of our codebase makes this a 
> necessity for keeping velocity while working unfortunately. So in this case I 
> would qualify that "it's not ready to be merged yet given our assumption that 
> all code in the codebase should serve an active immediate purpose, not due to 
> a lack of merge-level quality".
> 
> The approach of "hold the same bar for merges into a feature branch as trunk" 
> seems to be a compromise between Big Bang single commit drops and peppering 
> trunk with a lot of "as yet dormant" incremental code as a large feature is 
> built out. Not saying it's better or worse, just describing the contour of 
> the tradeoffs as I see them.
> 
>> - 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.
> This is an excellent point. The only mitigation I'd see for this would be an 
> additional review period or burden collectively before merge of a feature 
> branch into trunk once something has crossed a threshold of success as to be 
> included, or stepping away from a project where you don't have the cycles to 
> stay up to date and review and trust that the other committers working on the 
> project are making choices that are palatable and acceptable to you.
> 
> If all API decisions hit the dev ML and the architecture conforms generally 
> to the specification of the CEP, it seems to me that stepping back and 
> trusting your fellow committers to Do The Right Thing is the optimal (and 
> scalable) approach here?
> 
>> 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.
> I'd argue that there's 3 times to really invest in the quality of any 
> Cassandra release:
> 1. When we set agreed upon bars for quality we'll all hold ourselves 
> accou

Re: Merging CEP-15 to trunk

[Josh McKenzie]

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 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.
If this was a miscommunication on this instance rectifying it will of course 
require compromise from all parties. Good learning for future engagement and 
hopefully the outcome of this discussion is clearer norms as a project so we 
don't end up with this miscommunication in the future.

> 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
I think this is something we can either choose to make a formal requirement for 
feature branches in ASF git (all code that goes in has 2 committers hands on) 
or not. If folks want to work on other feature branches in other forks w/out 
this bar and then have a "mega review" at the end, I suppose that's their 
prerogative. Many of us that have been on the project for years have 
_significant emotional and psychological scars_ from that approach however, and 
multiple large efforts have failed at the "mega-review and merge" step. So I 
wouldn't advocate for that approach (and it's the only logical alternative I 
can think of to incremental bar of quality reinforcement throughout a work 
cycle on a large feature over time).

> 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
Right now we culturally tend to avoid merging code that doesn't do anything, 
for better or worse. We don't have a strong culture of either incremental merge 
in during development or of using the experimental flag for new features. Much 
of the tightly coupled nature of our codebase makes this a necessity for 
keeping velocity while working unfortunately. So in this case I would qualify 
that "it's not ready to be merged yet given our assumption that all code in the 
codebase should serve an active immediate purpose, not due to a lack of 
merge-level quality".

The approach of "hold the same bar for merges into a feature branch as trunk" 
seems to be a compromise between Big Bang single commit drops and peppering 
trunk with a lot of "as yet dormant" incremental code as a large feature is 
built out. Not saying it's better or worse, just describing the contour of the 
tradeoffs as I see them.

> - 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.
This is an excellent point. The only mitigation I'd see for this would be an 
additional review period or burden collectively before merge of a feature 
branch into trunk once something has crossed a threshold of success as to be 
included, or stepping away from a project where you don't have the cycles to 
stay up to date and review and trust that the other committers working on the 
project are making choices that are palatable and acceptable to you.

If all API decisions hit the dev ML and the architecture conforms generally to 
the specification of the CEP, it seems to me that stepping back and trusting 
your fellow committers to Do The Right Thing is the optimal (and scalable) 
approach here?

> 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.
I'd argue that there's 3 times to really invest in the quality of any Cassandra 
release:
1. When we set agreed upon bars for quality we'll all hold ourselves 
accountable to (CI, code style, test coverage, etc)
2. When we raise new committers
3. When we write or review code

I don't think it's sustainable to try and build processes that will bottleneck 
our throughput as a community to the finite availability of individuals if 
they're concerned about specific topics and want to be individually involved in 
specific code level changes. If folks feel like our current processes, CI 
infrastructure, and committer pool risks maintaining our bar of quality that's 
definitely something we should talk about in depth, as in my mind that's the 
backbone of us scaling stably as a project community.


On Tue, Jan 24, 2023, at 4:41 PM, Henrik Ingo wrote:
> Thanks Josh
> 
> Since you mentioned the CEP process, I should also mention one sentiment you 
> omitted, but worth stating explic

Re: Merging CEP-15 to trunk

[Caleb Rackliffe]

...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 first is (among other issues) a dependency for the second.

I'm also working through the trunk rebase right now, if anyone wants to
follow along...

1.) Pre-rebase
https://github.com/maedhroz/cassandra/commits/cep-15-accord-pre-1-24-rebase
https://app.circleci.com/pipelines/github/maedhroz/cassandra?branch=cep-15-accord-pre-1-24-rebase

2.) Post-rebase
https://github.com/maedhroz/cassandra/commits/cep-15-accord-post-1-24-rebase
https://app.circleci.com/pipelines/github/maedhroz/cassandra?branch=cep-15-accord-post-1-24-rebase

On Tue, Jan 24, 2023 at 3:42 PM Henrik Ingo 
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 
> 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 tr

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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

Re: Merging CEP-15 to trunk

[Josh McKenzie]

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

Re: Merging CEP-15 to trunk

[Caleb Rackliffe]

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

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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

Re: Merging CEP-15 to trunk

[Benedict]

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  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  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.henrikOn Tue, Jan 24, 2023 at 11:14 AM Benedict  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  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 Ingoc. +358 40 569 7354 w. www.datastax.com      

Re: Merging CEP-15 to trunk

[Ekaterina Dimitrova]

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

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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

  
  

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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
requirement for review. If we look at
https://cwiki.apache.org/confluence/display/CASSANDRA/Cassandra+Project+Governance
that would be point #3 in the process for code contributions.

On the other hand it seems clear that there is still active discussion
about the code (including at least one change request: better commented
code) and at least one committer has requested reasonable time to review.
These would be points #4 and #5 in the governance process, so clearly we
are in the state that code "must not be committed".

Presumably it makes sense to review only after the code has been better
documented, so the reasonable time might not have started yet? This is just
my understanding of the email discussion, I'm not participating in the
review myself.

I can also attest that at least for Jacek and Mick, as well as Ekaterina if
she participates, reviewing Accord will be the top priority of their day
job, so I believe the "reasonable time" criteria is clearly met in this
case.


If we step back from the process a bit, the above could also be summarized
as: It's not reasonable to expect that every committer would or should
track work happening in every feature branch. It's only natural that there
will need to be time for review at the point of merging to trunk.


PS: Personally I don't really believe in commit-then-merge. If the author
of the patch is committed to respond to review comments with high priority,
it shouldn't make a difference to them whether the code is committed before
or after the review. And of course if they aren't committed to work with
the reviewer with high priority, then what's the point of reviewing at all?
Since a reviewer is already obligated to do their part in a reasonable
time, it follows that merging after the review is the process where
incentives are aligned, AND there's no downside to the patch author.


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?
>
>
Why is there a difference whether the reviewer is a committer or a PMC
member?

henrik

-- 

Henrik Ingo

c. +358 40 569 7354

w. www.datastax.com

  
  

Re: Merging CEP-15 to trunk

[Benedict]

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

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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

Re: Merging CEP-15 to trunk

[Benedict]

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.

Squashing of commits is normal before merging a ticket, and does not typically 
incur an additional round of review - indeed, it’s not even clear what this 
would mean, since a squash has no visible effect to review.

This is not a question of moving fast, but a question of process. We have out 
of politeness highlighted the impending merge of a lot of work. This is an 
invitation to engage on the relevant tickets that already exist to represent 
the work, not an invitation to create novel adhoc procedural steps.


> On 23 Jan 2023, at 22:54, Mick Semb Wever  wrote:
> 
> 
>> 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 ninja-fix commits, 
> deserves an invite to reviewers.
> Any notion of merge-then-review isn't our community precedent.
> 
> I appreciate the desire to not "be left hanging" by creating a merge ticket 
> that requires a reviewer when no reviewer shows. And the desire to move 
> quickly on this.
> 
> I don't object if you wish to use this thread as that review process. On the 
> other hand, if you create the ticket I promise to be a reviewer of it, so as 
> not to delay.
> 
> 

Re: Merging CEP-15 to trunk

[Jeff Jirsa]

 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?


On Mon, Jan 23, 2023 at 2:54 PM Mick Semb Wever  wrote:

> 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 ninja-fix commits,
> deserves an invite to reviewers.
> Any notion of merge-then-review isn't our community precedent.
>
> I appreciate the desire to not "be left hanging" by creating a merge
> ticket that requires a reviewer when no reviewer shows. And the desire to
> move quickly on this.
>
> I don't object if you wish to use this thread as that review process. On
> the other hand, if you create the ticket I promise to be a reviewer of it,
> so as not to delay.
>
>
>

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

>
> 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 ninja-fix commits,
deserves an invite to reviewers.
Any notion of merge-then-review isn't our community precedent.

I appreciate the desire to not "be left hanging" by creating a merge ticket
that requires a reviewer when no reviewer shows. And the desire to move
quickly on this.

I don't object if you wish to use this thread as that review process. On
the other hand, if you create the ticket I promise to be a reviewer of it,
so as not to delay.

Re: Merging CEP-15 to trunk

[Aleksey Yeshchenko]

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 happens / if anyone is interested. But if nobody expresses any concerns 
> or asks for time to look into something specific, then I agree that the 
> reviews that have already happened in the feature branch are sufficient and  
> there isn't a need for a new full blown review.

That sounds perfectly reasonable to me.

Truth is, next major release is pretty far away. This is not an attempt to 
fast-track a major change right before a release vote. Anything can be 
reverted, anything can be modified, and many things will be before now and then 
- there is a pretty long list of work remaining to be done.

The CEP was voted on, development is progressing along the lines of the CEP, 
and any extra review on top of the mandated (and objectively already cleared) 
bar can be performed on trunk commits just as easily after the merge. Many 
months ahead for any volunteers to take a look at the work already done and the 
work yet to come and raise any issues.

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.

> On 20 Jan 2023, at 15:34, Henrik Ingo  wrote:
> 
> 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 limits.
> 
> Since most contributors would not have paid attention to a feature branch, 
> the result is that that additional review happens now. If it happens / if 
> anyone is interested. But if nobody expresses any concerns or asks for time 
> to look into something specific, then I agree that the reviews that have 
> already happened in the feature branch are sufficient and  there isn't a need 
> for a new full blown review.
> 
> As far as I can tell, this email thread is exactly that process and I imagine 
> was at least one of the reasons to send this heads up email?
> 
> henrik
> 
> On Fri, Jan 20, 2023 at 5:23 PM Aleksey Yeshchenko  > wrote:
>> 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.
>> 
>>> On 20 Jan 2023, at 12:31, Mick Semb Wever >> > wrote:
>>> 
 
 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 pass".  
>>> Either way I believe it is a good thing.
>> 
> 
> 
> -- 
> 
> Henrik Ingo
> c. +358 40 569 7354 
> w. www.datastax.com 
>        
>    

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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


Yes yes. But we do catch things late, and the eyes on a merge would have a
different PoV than original reviews, and that can be helpful. And yes,
we can also review things post-commit if we like. I'm not saying it has to
be done, or that our rules enforce it, just that I think it would be
helpful to offer the invite for more eyeballs (and provide the hygiene in
jira).
Looking through the cep-15-accord branch there's >10 ninja-fixes.
Won't these get cleaned up?
And if so, shouldn't changes be open to another review round?

It just seems like a more congenial manner to collaborate 🤷🏻‍♀️
And maybe no one is interested in such a second pass pre-merge review. idk.

Re: Merging CEP-15 to trunk

[Henrik Ingo]

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
limits.

Since most contributors would not have paid attention to a feature branch,
the result is that that additional review happens now. If it happens / if
anyone is interested. But if nobody expresses any concerns or asks for time
to look into something specific, then I agree that the reviews that have
already happened in the feature branch are sufficient and  there isn't a
need for a new full blown review.

As far as I can tell, this email thread is exactly that process and I
imagine was at least one of the reasons to send this heads up email?

henrik

On Fri, Jan 20, 2023 at 5:23 PM Aleksey Yeshchenko 
wrote:

> 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.
>
> On 20 Jan 2023, at 12:31, Mick Semb Wever  wrote:
>
>
>> 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 pass".
> Either way I believe it is a good thing.
>
>
>

-- 

Henrik Ingo

c. +358 40 569 7354

w. www.datastax.com

  
  

Re: Merging CEP-15 to trunk

[Aleksey Yeshchenko]

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.

> On 20 Jan 2023, at 12:31, Mick Semb Wever  wrote:
> 
>> 
>> 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 pass".  
> Either way I believe it is a good thing.

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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 pass".
Either way I believe it is a good thing.



> I think the existing epics are probably more natural tickets to reference
> in the merge, eg 17091 and 17092.
>


If _all_ tickets in that epic are being merged, sure. Otherwise how can I
see what resolved tickets are in trunk and what resolved tickets are not…

I would rather not have to be working on accord actively to be able to see
this stuff quickly.

Re: Merging CEP-15 to trunk

[Benedict]

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  wrote:On Tue, 17 Jan 2023 at 10:29, Benedict  wrote:but the pre-commit gateway here is higher than the previous tickets being worked onWhich 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… 17103, 17109, 18041, 18056, 18057, 18087, 17719, 18154, 18142, 18142.All these tickets are resolved but have not been merged to trunk, and they have no fixVersion.Assuming that it won't be a merge as-is (e.g. ninja-fixes will get squashed), i think a task ticket for the clean up of the feature branch and merge of it to trunk warrants a separate ticket. Such a ticket also helps with the admin (linking to the tickets the merge is bringing in).  

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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… 17103, 17109, 18041, 18056, 18057, 18087, 17719, 18154,
18142, 18142.

All these tickets are resolved but have not been merged to trunk, and they
have no fixVersion.

Assuming that it won't be a merge as-is (e.g. ninja-fixes will get
squashed), i think a task ticket for the clean up of the feature branch and
merge of it to trunk warrants a separate ticket. Such a ticket also helps
with the admin (linking to the tickets the merge is bringing in).

Re: Merging CEP-15 to trunk

[Benedict]

> 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 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, but the 
> pre-commit gateway here is higher than the previous tickets being worked on. 
> API changes, pre-commit test results, and high (/entry) level comments, all 
> deserve any extra eyeballs available.
> 

Re: Merging CEP-15 to trunk

[Mick Semb Wever]

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, but the
pre-commit gateway here is higher than the previous tickets being worked
on. API changes, pre-commit test results, and high (/entry) level comments,
all deserve any extra eyeballs available.

Re: Merging CEP-15 to trunk

[Benedict]

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 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 whether the implementation in Cassandra conforms the API requirements. Given it is going to be a possibility for others to try Accord before the release, it would be good to create some CQL syntax documentation, something like a chapter in https://cassandra.apache.org/doc/latest/cassandra/cql/index.html but for unreleasedCassandra version or a blog post, so that the syntax is known to the users and they can quickly get into speed, hopefully reporting any problems soon.- - -- --- -  -Jacek LewandowskiOn Mon, 16 Jan 2023 at 17:52, Benedict  wrote: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 buried in one of the many threads we’ve had about related topics on releases and development. We’ve definitely discussed feature branches before, and I recall discussing a goal of merging ~quarterly. But perhaps like most sub topics it didn’t get enough visibility, in which case this thread I suppose can serve as a dedicated rehash and we can formalise whatever falls out.In theory as Jeremiah says there’s only the normal merge criteria. But that includes nobody saying no to a piece of work or raising concerns, and advertising the opportunity to say no is important for that IMO.On 16 Jan 2023, at 16:36, J. D. Jordan  wrote: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 behind a feature flag.-JeremiahOn Jan 16, 2023, at 10:30 AM, Josh McKenzie  wrote: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 to2. 2 committers eyes on (author + reviewer or 2 reviewers, etc)3. Disabled by default w/flag to enableSo really only the 3rd thing is different right? Probably ought to add an informal step 4 which Benedict is doing here which is "hit the dev ML w/a DISCUSS thread about the upcoming merge so it's on people's radar and they can coordinate".On Mon, Jan 16, 2023, at 11:08 AM, Benedict wrote: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 haven’t even noticed the feature branch developing.The relevant standard for merging a feature branch, if we want to rehash that, is that it is feature- and bug-neutral by default, ie that a release could be cut afterwards while maintaining our usual quality standards, and that the feature is disabled by default, yes. It is not however feature-complete or production read as a feature; that would prevent any incremental merging of feature development.> On 16 Jan 2023, at 15:57, J. D. Jordan  wrote:> > 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 not?> Do all the tests pass?> Has there been review and +1 by two committer?> > If the code in the feature branch meets all of the merging criteria of the project then I see no reason to keep it in a feature branch for ever.> > -Jeremiah> > >> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:>> >> Hi Everyone, I hope you all had a lovely holiday period. >> >> Those who have been following along will have seen a steady drip of progress into the cep-15-accord feature branch over the past year. We originally discussed that feature branches would merge periodically into trunk, and we are long overdue. With the release of 4.1, it’s time to rectify that. >> >> Barring complaints, I hope to merge the current state 

Re: Merging CEP-15 to trunk

[Jacek Lewandowski]

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 whether the implementation in
Cassandra conforms the API requirements.

Given it is going to be a possibility for others to try Accord before the
release, it would be good to create some CQL syntax
documentation, something like a chapter in
https://cassandra.apache.org/doc/latest/cassandra/cql/index.html but for
unreleased
Cassandra version or a blog post, so that the syntax is known to the users
and they can quickly get into speed, hopefully
reporting any problems soon.

- - -- --- -  -
Jacek Lewandowski


On Mon, 16 Jan 2023 at 17:52, Benedict  wrote:

> 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 buried in one of the many threads we’ve had about related
> topics on releases and development. We’ve definitely discussed feature
> branches before, and I recall discussing a goal of merging ~quarterly. But
> perhaps like most sub topics it didn’t get enough visibility, in which case
> this thread I suppose can serve as a dedicated rehash and we can formalise
> whatever falls out.
>
> In theory as Jeremiah says there’s only the normal merge criteria. But
> that includes nobody saying no to a piece of work or raising concerns, and
> advertising the opportunity to say no is important for that IMO.
>
> On 16 Jan 2023, at 16:36, J. D. Jordan  wrote:
>
> 
> 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
> behind a feature flag.
>
> -Jeremiah
>
> On Jan 16, 2023, at 10:30 AM, Josh McKenzie  wrote:
>
> 
> 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 (author + reviewer or 2 reviewers, etc)
> 3. Disabled by default w/flag to enable
>
> So really only the 3rd thing is different right? Probably ought to add an
> informal step 4 which Benedict is doing here which is "hit the dev ML w/a
> DISCUSS thread about the upcoming merge so it's on people's radar and they
> can coordinate".
>
> On Mon, Jan 16, 2023, at 11:08 AM, Benedict wrote:
>
> 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 haven’t
> even noticed the feature branch developing.
>
> The relevant standard for merging a feature branch, if we want to rehash
> that, is that it is feature- and bug-neutral by default, ie that a release
> could be cut afterwards while maintaining our usual quality standards, and
> that the feature is disabled by default, yes. It is not however
> feature-complete or production read as a feature; that would prevent any
> incremental merging of feature development.
>
> > On 16 Jan 2023, at 15:57, J. D. Jordan 
> wrote:
> >
> > 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 not?
> > Do all the tests pass?
> > Has there been review and +1 by two committer?
> >
> > If the code in the feature branch meets all of the merging criteria of
> the project then I see no reason to keep it in a feature branch for ever.
> >
> > -Jeremiah
> >
> >
> >> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:
> >>
> >> Hi Everyone, I hope you all had a lovely holiday period.
> >>
> >> Those who have been following along will have seen a steady drip of
> progress into the cep-15-accord feature branch over the past year. We
> originally discussed that feature branches would merge periodically into
> trunk, and we are long overdue. With the release of 4.1, it’s time to
> rectify that.
> >>
> >> Barring complaints, I hope to merge the current state to trunk within a
> couple of weeks. This remains a work in progress, but will permit users to
> experiment with the alpha version of Accord a

Re: Merging CEP-15 to trunk

[Benedict]

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 buried in one of the many threads we’ve had about related topics on releases and development. We’ve definitely discussed feature branches before, and I recall discussing a goal of merging ~quarterly. But perhaps like most sub topics it didn’t get enough visibility, in which case this thread I suppose can serve as a dedicated rehash and we can formalise whatever falls out.In theory as Jeremiah says there’s only the normal merge criteria. But that includes nobody saying no to a piece of work or raising concerns, and advertising the opportunity to say no is important for that IMO.On 16 Jan 2023, at 16:36, J. D. Jordan  wrote: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 behind a feature flag.-JeremiahOn Jan 16, 2023, at 10:30 AM, Josh McKenzie  wrote: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 to2. 2 committers eyes on (author + reviewer or 2 reviewers, etc)3. Disabled by default w/flag to enableSo really only the 3rd thing is different right? Probably ought to add an informal step 4 which Benedict is doing here which is "hit the dev ML w/a DISCUSS thread about the upcoming merge so it's on people's radar and they can coordinate".On Mon, Jan 16, 2023, at 11:08 AM, Benedict wrote: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 haven’t even noticed the feature branch developing.The relevant standard for merging a feature branch, if we want to rehash that, is that it is feature- and bug-neutral by default, ie that a release could be cut afterwards while maintaining our usual quality standards, and that the feature is disabled by default, yes. It is not however feature-complete or production read as a feature; that would prevent any incremental merging of feature development.> On 16 Jan 2023, at 15:57, J. D. Jordan  wrote:> > 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 not?> Do all the tests pass?> Has there been review and +1 by two committer?> > If the code in the feature branch meets all of the merging criteria of the project then I see no reason to keep it in a feature branch for ever.> > -Jeremiah> > >> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:>> >> Hi Everyone, I hope you all had a lovely holiday period. >> >> Those who have been following along will have seen a steady drip of progress into the cep-15-accord feature branch over the past year. We originally discussed that feature branches would merge periodically into trunk, and we are long overdue. With the release of 4.1, it’s time to rectify that. >> >> Barring complaints, I hope to merge the current state to trunk within a couple of weeks. This remains a work in progress, but will permit users to experiment with the alpha version of Accord and provide feedback, as well as phase the changes to trunk.

Re: Merging CEP-15 to trunk

[J. D. Jordan]

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 behind a feature flag.-JeremiahOn Jan 16, 2023, at 10:30 AM, Josh McKenzie  wrote: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 to2. 2 committers eyes on (author + reviewer or 2 reviewers, etc)3. Disabled by default w/flag to enableSo really only the 3rd thing is different right? Probably ought to add an informal step 4 which Benedict is doing here which is "hit the dev ML w/a DISCUSS thread about the upcoming merge so it's on people's radar and they can coordinate".On Mon, Jan 16, 2023, at 11:08 AM, Benedict wrote: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 haven’t even noticed the feature branch developing.The relevant standard for merging a feature branch, if we want to rehash that, is that it is feature- and bug-neutral by default, ie that a release could be cut afterwards while maintaining our usual quality standards, and that the feature is disabled by default, yes. It is not however feature-complete or production read as a feature; that would prevent any incremental merging of feature development.> On 16 Jan 2023, at 15:57, J. D. Jordan  wrote:> > 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 not?> Do all the tests pass?> Has there been review and +1 by two committer?> > If the code in the feature branch meets all of the merging criteria of the project then I see no reason to keep it in a feature branch for ever.> > -Jeremiah> > >> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:>> >> Hi Everyone, I hope you all had a lovely holiday period. >> >> Those who have been following along will have seen a steady drip of progress into the cep-15-accord feature branch over the past year. We originally discussed that feature branches would merge periodically into trunk, and we are long overdue. With the release of 4.1, it’s time to rectify that. >> >> Barring complaints, I hope to merge the current state to trunk within a couple of weeks. This remains a work in progress, but will permit users to experiment with the alpha version of Accord and provide feedback, as well as phase the changes to trunk.

Re: Merging CEP-15 to trunk

[Josh McKenzie]

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 (author + reviewer or 2 reviewers, etc)
3. Disabled by default w/flag to enable

So really only the 3rd thing is different right? Probably ought to add an 
informal step 4 which Benedict is doing here which is "hit the dev ML w/a 
DISCUSS thread about the upcoming merge so it's on people's radar and they can 
coordinate".

On Mon, Jan 16, 2023, at 11:08 AM, Benedict wrote:
> 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 haven’t even 
> noticed the feature branch developing.
> 
> The relevant standard for merging a feature branch, if we want to rehash 
> that, is that it is feature- and bug-neutral by default, ie that a release 
> could be cut afterwards while maintaining our usual quality standards, and 
> that the feature is disabled by default, yes. It is not however 
> feature-complete or production read as a feature; that would prevent any 
> incremental merging of feature development.
> 
> > On 16 Jan 2023, at 15:57, J. D. Jordan  wrote:
> > 
> > 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 not?
> > Do all the tests pass?
> > Has there been review and +1 by two committer?
> > 
> > If the code in the feature branch meets all of the merging criteria of the 
> > project then I see no reason to keep it in a feature branch for ever.
> > 
> > -Jeremiah
> > 
> > 
> >> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:
> >> 
> >> Hi Everyone, I hope you all had a lovely holiday period. 
> >> 
> >> Those who have been following along will have seen a steady drip of 
> >> progress into the cep-15-accord feature branch over the past year. We 
> >> originally discussed that feature branches would merge periodically into 
> >> trunk, and we are long overdue. With the release of 4.1, it’s time to 
> >> rectify that. 
> >> 
> >> Barring complaints, I hope to merge the current state to trunk within a 
> >> couple of weeks. This remains a work in progress, but will permit users to 
> >> experiment with the alpha version of Accord and provide feedback, as well 
> >> as phase the changes to trunk.
> 
> 

Re: Merging CEP-15 to trunk

[Benedict]

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 haven’t even 
noticed the feature branch developing.

The relevant standard for merging a feature branch, if we want to rehash that, 
is that it is feature- and bug-neutral by default, ie that a release could be 
cut afterwards while maintaining our usual quality standards, and that the 
feature is disabled by default, yes. It is not however feature-complete or 
production read as a feature; that would prevent any incremental merging of 
feature development.

> On 16 Jan 2023, at 15:57, J. D. Jordan  wrote:
> 
> 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 not?
> Do all the tests pass?
> Has there been review and +1 by two committer?
> 
> If the code in the feature branch meets all of the merging criteria of the 
> project then I see no reason to keep it in a feature branch for ever.
> 
> -Jeremiah
> 
> 
>> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:
>> 
>> Hi Everyone, I hope you all had a lovely holiday period. 
>> 
>> Those who have been following along will have seen a steady drip of progress 
>> into the cep-15-accord feature branch over the past year. We originally 
>> discussed that feature branches would merge periodically into trunk, and we 
>> are long overdue. With the release of 4.1, it’s time to rectify that. 
>> 
>> Barring complaints, I hope to merge the current state to trunk within a 
>> couple of weeks. This remains a work in progress, but will permit users to 
>> experiment with the alpha version of Accord and provide feedback, as well as 
>> phase the changes to trunk.

Re: Merging CEP-15 to trunk

[J. D. Jordan]

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 not?
Do all the tests pass?
Has there been review and +1 by two committer?

If the code in the feature branch meets all of the merging criteria of the 
project then I see no reason to keep it in a feature branch for ever.

-Jeremiah


> On Jan 16, 2023, at 3:21 AM, Benedict  wrote:
> 
> Hi Everyone, I hope you all had a lovely holiday period. 
> 
> Those who have been following along will have seen a steady drip of progress 
> into the cep-15-accord feature branch over the past year. We originally 
> discussed that feature branches would merge periodically into trunk, and we 
> are long overdue. With the release of 4.1, it’s time to rectify that. 
> 
> Barring complaints, I hope to merge the current state to trunk within a 
> couple of weeks. This remains a work in progress, but will permit users to 
> experiment with the alpha version of Accord and provide feedback, as well as 
> phase the changes to trunk.