Re: PR, and a question

[David Capwell]

I prefer many smaller PRs than a uber PR to rule them all, but others feel uber 
is the only way.

Honestly, this topic I delegate to the people actually reviewing and involved 
in the PR.  If you are refactoring in isolation and the implementation will be 
its own PR, to me this is fine as long as your reviewers are cool with it.

One thing to look out for is dead code.  Refactors with the intent to have the 
implementation come later are at higher risk to add dead code, so make sure 
your implementation patch cleans up anything no longer needed in the refactor.

> On Oct 29, 2025, at 5:20 AM, Josh McKenzie  wrote:
> 
> I'm of the opinion that describing what it does discretely today and, if it's 
> a refactor that tidies or formalizes APIs, explaining the intent of what you 
> want to enable in the future is both valid and valuable. Just because you 
> might not personally get to that work (priorities change) doesn't mean 
> someone else might not have similar interests.
> 
> We've long gone back and forth on the "don't prematurely formalize an 
> interface" topic. While I'm sympathetic to both sides of it, if it's a step 
> in a larger block of intended work it helps to call out why you're doing what 
> you're doing and I think the value in getting it tested and integrated 
> incrementally justifies the slight cognitive cost of expanding the scope for 
> a reviewer to consider the broader context in which it is intended to be used 
> going forward.
> 
> As you can probably tell though, opinions kind of range on this one Joel so 
> it's something of a "up to you" scenario. :)
> 
> On Tue, Oct 28, 2025, at 3:09 PM, Joel Shepherd wrote:
>> Hi Stefan - Thanks for the feedback: appreciate it!
>> 
>> -- Joel.
>> 
>> On 10/28/2025 2:17 AM, Štefan Miklošovič wrote:
>> 
>> > I went through your PR very quickly. If you want to add to trunk, any
>> > change for that matter, what is important is that it is self-contained
>> > change / feature / fix nothing else depends on if we ever go to
>> > release it.
>> >
>> > If your patch is going to be merged tomorrow and the day after that we
>> > release trunk, is there anything else which needs to happen to your
>> > patch in order to do so? The answer is, as I looked at it, no. Your
>> > patch is a standalone refactorisation of the code which, once done,
>> > just enables you to incorporate more code / functionality on top of it
>> > more comfortably.
>> >
>> >  From my perspective what you did is correct and valid. I also do not
>> > think it is low value reshuffling either.
>> >
>> > However, what I would do is that I would rename what your patch does.
>> > "Factoring out assumption of a single node-wide authenticator" is in
>> > my books not a good description / title, because at the time you
>> > implemented this, there was technically no functionality where we
>> > would have multiple node-wide authenticators. Just describe what your
>> > patch does, not _what it is going to enable when merged_. Because you
>> > never know if that follow-up change will ever come.
>> >
>> > In your case it would be like: "Enable IAuthenticator to declare
>> > supported and alterable role options".
>> >
>> > On Mon, Oct 27, 2025 at 7:22 PM Joel Shepherd > > > wrote:
>> >> Hi C* devs - Small request and a slightly larger question ...
>> >>
>> >> I have a small PR out as a first step towards (CEP-50: Auth negotiation)
>> >> - Refactoring out  most assumptions of a single authenticator outside of
>> >> Config and DatabaseDescriptor:
>> >> https://github.com/apache/cassandra/pull/4427  ... gentle ping if anyone
>> >> wants to take a look.
>> >>
>> >> Larger question - I'm used to a workflow where devs submit a number of
>> >> small CRs/PRs building up a larger piece of work, instead of posting one
>> >> or two big reviews at the end. I want to verify what this community
>> >> prefers. One advantage I can see to fewer, bigger PRs is that there is
>> >> more context to evaluate the overall approach with. E.g., with the PR
>> >> mentioned above, it might not be obvious from the changes in the PR what
>> >> the larger intent is, so it could look like low-value code reshuffling,
>> >> rather than a building block.
>> >>
>> >> So, what working style would be better for you?
>> >>
>> >> Thanks -- Joel.
>> >>
>> >>
>> 
> 

Re: PR, and a question

[Josh McKenzie]

I'm of the opinion that describing what it does discretely today and, if it's a 
refactor that tidies or formalizes APIs, explaining the intent of what you want 
to enable in the future is both valid and valuable. Just because you might not 
personally get to that work (priorities change) doesn't mean someone else might 
not have similar interests.

We've long gone back and forth on the "don't prematurely formalize an 
interface" topic. While I'm sympathetic to both sides of it, if it's a step in 
a larger block of intended work it helps to call out why you're doing what 
you're doing and I think the value in getting it tested and integrated 
incrementally justifies the slight cognitive cost of expanding the scope for a 
reviewer to consider the broader context in which it is intended to be used 
going forward.

As you can probably tell though, opinions kind of range on this one Joel so 
it's something of a "up to you" scenario. :)

On Tue, Oct 28, 2025, at 3:09 PM, Joel Shepherd wrote:
> Hi Stefan - Thanks for the feedback: appreciate it!
> 
> -- Joel.
> 
> On 10/28/2025 2:17 AM, Štefan Miklošovič wrote:
> 
> > I went through your PR very quickly. If you want to add to trunk, any
> > change for that matter, what is important is that it is self-contained
> > change / feature / fix nothing else depends on if we ever go to
> > release it.
> >
> > If your patch is going to be merged tomorrow and the day after that we
> > release trunk, is there anything else which needs to happen to your
> > patch in order to do so? The answer is, as I looked at it, no. Your
> > patch is a standalone refactorisation of the code which, once done,
> > just enables you to incorporate more code / functionality on top of it
> > more comfortably.
> >
> >  From my perspective what you did is correct and valid. I also do not
> > think it is low value reshuffling either.
> >
> > However, what I would do is that I would rename what your patch does.
> > "Factoring out assumption of a single node-wide authenticator" is in
> > my books not a good description / title, because at the time you
> > implemented this, there was technically no functionality where we
> > would have multiple node-wide authenticators. Just describe what your
> > patch does, not _what it is going to enable when merged_. Because you
> > never know if that follow-up change will ever come.
> >
> > In your case it would be like: "Enable IAuthenticator to declare
> > supported and alterable role options".
> >
> > On Mon, Oct 27, 2025 at 7:22 PM Joel Shepherd  wrote:
> >> Hi C* devs - Small request and a slightly larger question ...
> >>
> >> I have a small PR out as a first step towards (CEP-50: Auth negotiation)
> >> - Refactoring out  most assumptions of a single authenticator outside of
> >> Config and DatabaseDescriptor:
> >> https://github.com/apache/cassandra/pull/4427  ... gentle ping if anyone
> >> wants to take a look.
> >>
> >> Larger question - I'm used to a workflow where devs submit a number of
> >> small CRs/PRs building up a larger piece of work, instead of posting one
> >> or two big reviews at the end. I want to verify what this community
> >> prefers. One advantage I can see to fewer, bigger PRs is that there is
> >> more context to evaluate the overall approach with. E.g., with the PR
> >> mentioned above, it might not be obvious from the changes in the PR what
> >> the larger intent is, so it could look like low-value code reshuffling,
> >> rather than a building block.
> >>
> >> So, what working style would be better for you?
> >>
> >> Thanks -- Joel.
> >>
> >>
> 

Re: PR, and a question

[Joel Shepherd]

Hi Stefan - Thanks for the feedback: appreciate it!

-- Joel.

On 10/28/2025 2:17 AM, Štefan Miklošovič wrote:


I went through your PR very quickly. If you want to add to trunk, any
change for that matter, what is important is that it is self-contained
change / feature / fix nothing else depends on if we ever go to
release it.

If your patch is going to be merged tomorrow and the day after that we
release trunk, is there anything else which needs to happen to your
patch in order to do so? The answer is, as I looked at it, no. Your
patch is a standalone refactorisation of the code which, once done,
just enables you to incorporate more code / functionality on top of it
more comfortably.

 From my perspective what you did is correct and valid. I also do not
think it is low value reshuffling either.

However, what I would do is that I would rename what your patch does.
"Factoring out assumption of a single node-wide authenticator" is in
my books not a good description / title, because at the time you
implemented this, there was technically no functionality where we
would have multiple node-wide authenticators. Just describe what your
patch does, not _what it is going to enable when merged_. Because you
never know if that follow-up change will ever come.

In your case it would be like: "Enable IAuthenticator to declare
supported and alterable role options".

On Mon, Oct 27, 2025 at 7:22 PM Joel Shepherd  wrote:

Hi C* devs - Small request and a slightly larger question ...

I have a small PR out as a first step towards (CEP-50: Auth negotiation)
- Refactoring out  most assumptions of a single authenticator outside of
Config and DatabaseDescriptor:
https://github.com/apache/cassandra/pull/4427  ... gentle ping if anyone
wants to take a look.

Larger question - I'm used to a workflow where devs submit a number of
small CRs/PRs building up a larger piece of work, instead of posting one
or two big reviews at the end. I want to verify what this community
prefers. One advantage I can see to fewer, bigger PRs is that there is
more context to evaluate the overall approach with. E.g., with the PR
mentioned above, it might not be obvious from the changes in the PR what
the larger intent is, so it could look like low-value code reshuffling,
rather than a building block.

So, what working style would be better for you?

Thanks -- Joel.

Re: PR, and a question

[Joel Shepherd]

Hi Ekaterina - Thanks for the suggestions.

On 10/27/2025 11:31 AM, Ekaterina Dimitrova wrote:

Hi Joel,
I think this is very reasonable question. I would say - it depends.

Incremental small changes are always welcome - easier and quicker to 
review and less prone to miss something. If they are self contained 
and we can release anytime with them committed, even if the CEP is not 
finished yet - then they can be committed to trunk, once reviewed etc. 
Otherwise, we can use feature branch as we did with TCM and Accord, 
for example.


Thinking about it, this kind of overlaps with the ongoing discussion 
about forks, backports, etc., and the desire to keep trunk releasable at 
any moment. Ideally it would be possible to slip in a large change via a 
bunch of little commits that are always releasable, but the bigger the 
change the riskier that sounds. (Each little commit carries non-zero 
risk of introducing something bad in trunk; a bunch of little commits 
probably makes the risk add up.) I'll consider a feature branch, unless 
there's strong opinions against.


This helps to split the work in manageable PR sizes and also more 
people can collaborate on the CEP, working on the feature.


“E.g., with the PR
mentioned above, it might not be obvious from the changes in the PR what
the larger intent is, so it could look like low-value code reshuffling,
rather than a building block.”
The commit will mention a ticket number part of CEP epic (I assume you 
will have one to cover the different parts you will commit 
separately), to make it clear what its goal is.


Hope this makes sense,


Yes it does: thanks -- Joel.



On Mon, 27 Oct 2025 at 14:23, Joel Shepherd  wrote:

Hi C* devs - Small request and a slightly larger question ...

I have a small PR out as a first step towards (CEP-50: Auth
negotiation)
- Refactoring out  most assumptions of a single authenticator
outside of
Config and DatabaseDescriptor:
https://github.com/apache/cassandra/pull/4427 ... gentle ping if
anyone
wants to take a look.

Larger question - I'm used to a workflow where devs submit a
number of
small CRs/PRs building up a larger piece of work, instead of
posting one
or two big reviews at the end. I want to verify what this community
prefers. One advantage I can see to fewer, bigger PRs is that
there is
more context to evaluate the overall approach with. E.g., with the PR
mentioned above, it might not be obvious from the changes in the
PR what
the larger intent is, so it could look like low-value code
reshuffling,
rather than a building block.

So, what working style would be better for you?

Thanks -- Joel.

Re: PR, and a question

[Štefan Miklošovič]

I went through your PR very quickly. If you want to add to trunk, any
change for that matter, what is important is that it is self-contained
change / feature / fix nothing else depends on if we ever go to
release it.

If your patch is going to be merged tomorrow and the day after that we
release trunk, is there anything else which needs to happen to your
patch in order to do so? The answer is, as I looked at it, no. Your
patch is a standalone refactorisation of the code which, once done,
just enables you to incorporate more code / functionality on top of it
more comfortably.

>From my perspective what you did is correct and valid. I also do not
think it is low value reshuffling either.

However, what I would do is that I would rename what your patch does.
"Factoring out assumption of a single node-wide authenticator" is in
my books not a good description / title, because at the time you
implemented this, there was technically no functionality where we
would have multiple node-wide authenticators. Just describe what your
patch does, not _what it is going to enable when merged_. Because you
never know if that follow-up change will ever come.

In your case it would be like: "Enable IAuthenticator to declare
supported and alterable role options".

On Mon, Oct 27, 2025 at 7:22 PM Joel Shepherd  wrote:
>
> Hi C* devs - Small request and a slightly larger question ...
>
> I have a small PR out as a first step towards (CEP-50: Auth negotiation)
> - Refactoring out  most assumptions of a single authenticator outside of
> Config and DatabaseDescriptor:
> https://github.com/apache/cassandra/pull/4427  ... gentle ping if anyone
> wants to take a look.
>
> Larger question - I'm used to a workflow where devs submit a number of
> small CRs/PRs building up a larger piece of work, instead of posting one
> or two big reviews at the end. I want to verify what this community
> prefers. One advantage I can see to fewer, bigger PRs is that there is
> more context to evaluate the overall approach with. E.g., with the PR
> mentioned above, it might not be obvious from the changes in the PR what
> the larger intent is, so it could look like low-value code reshuffling,
> rather than a building block.
>
> So, what working style would be better for you?
>
> Thanks -- Joel.
>
>

Re: PR, and a question

[Ekaterina Dimitrova]

Hi Joel,
I think this is very reasonable question. I would say - it depends.

Incremental small changes are always welcome - easier and quicker to review
and less prone to miss something. If they are self contained and we can
release anytime with them committed, even if the CEP is not finished yet -
then they can be committed to trunk, once reviewed etc. Otherwise, we can
use feature branch as we did with TCM and Accord, for example.

This helps to split the work in manageable PR sizes and also more people
can collaborate on the CEP, working on the feature.

“E.g., with the PR
mentioned above, it might not be obvious from the changes in the PR what
the larger intent is, so it could look like low-value code reshuffling,
rather than a building block.”
The commit will mention a ticket number part of CEP epic (I assume you will
have one to cover the different parts you will commit separately), to make
it clear what its goal is.

Hope this makes sense,

Best regards,
Ekaterina


On Mon, 27 Oct 2025 at 14:23, Joel Shepherd  wrote:

> Hi C* devs - Small request and a slightly larger question ...
>
> I have a small PR out as a first step towards (CEP-50: Auth negotiation)
> - Refactoring out  most assumptions of a single authenticator outside of
> Config and DatabaseDescriptor:
> https://github.com/apache/cassandra/pull/4427  ... gentle ping if anyone
> wants to take a look.
>
> Larger question - I'm used to a workflow where devs submit a number of
> small CRs/PRs building up a larger piece of work, instead of posting one
> or two big reviews at the end. I want to verify what this community
> prefers. One advantage I can see to fewer, bigger PRs is that there is
> more context to evaluate the overall approach with. E.g., with the PR
> mentioned above, it might not be obvious from the changes in the PR what
> the larger intent is, so it could look like low-value code reshuffling,
> rather than a building block.
>
> So, what working style would be better for you?
>
> Thanks -- Joel.
>
>
>