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 <[email protected]> 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. > >> > >> >
