There’s a pretty simple solution here - breaking it up into several smaller patches.
* Any changes should include tests that validate the checks are used correctly. * It should also alleviate any issues with code conflicts and rebasing as the merges would happen slowly over time rather than all at once. * If there’s two committers willing to spend time and work with OP on this, that should be enough to move it forward. * There's a thread on user@ right now [1] where someone *just* ran into this issue, so I'd say addressing that one is a reasonable starting point. [1] https://lists.apache.org/thread/ykkwhjdpgyqzw5xtol4v5ysz664bxxl3 Jon On Fri, May 9, 2025 at 12:16 PM C. Scott Andreas <sc...@paradoxica.net> wrote: > My thinking is most closely aligned with Blake and Benedict’s views here. > > For the specific refactor in question, I support adoption of the language > feature for new code or to cut existing code over to the new syntax as > changes are made to the respective areas of the codebase. But I don’t > support a sweeping project-wide refactor on trunk in this case. > > Here is my thinking: > > - If there are 2000 target sites for the refactor, that means this is > going to be a 5000+ line diff. > - The safety improvement here is marginal but nonzero. > - If we have a 5000 line refactor, it should accomplish a significant and > compelling purpose in the project. > - Any execution of the refactor will require manual review of each of > those 2000 refactor sites on the part of the implementer and two reviewers. > - Since the check is compile-time, we’d learn that by the initial refactor > the first time it’s compiled, and we short-circuit to having gained 100% of > the value by being able to fix the broken callsites. > - The act of that per-call site review would inform us as to whether we > had incorrect casts; and we would immediately achieve the value of the > “safer” approach by having identified the bugs. > - 2x reviewer coverage for a 5000 line patch set is a significant > commitment of reviewer resources. These reviewer resources have significant > opportunity cost and can put to a better purpose. > - Blake/others mention that such refactors create conflicts when bug fixes > are backported to previous releases, requiring refactors of those rebased > patches to bring fixes to versions that predate the large refactor. > > I think this is a good language feature. I think we should use it. I think > it’d be completely reasonable to cut existing syntax over to it as we make > changes to the respective subsystems. > > But I wouldn’t do a big bang refactor in this case. The juice isn’t worth > the squeeze for me. > > - Scott > > On May 9, 2025, at 11:33 AM, Blake Eggleston <bl...@ultrablake.com> wrote: > > > > No one is treating the codebase like a house of cards that can’t be > touched. > > In this case I think the cost/risk of doing this change outweighs the > potential benefits the project might see from it. Josh counts ~2000 > instances where we’re casting objects so we’re talking about a > not-insignificant change which may introduce it’s own bugs. Even if no new > bugs are introduced, this will be an refactor annoyance for projects in > development, but the real concern I have with any large change is how it > complicates the process of fixing bugs across versions. On the other hand, > I don’t think that incorrectly casting objects has historically been a > source of pain for us, so it seems like the benefit would be small if any. > > On Fri, May 9, 2025, at 10:38 AM, Jon Haddad wrote: > > Why not? > > Personally, I hate the idea of treating a codebase (any codebase) like a > house of cards that can't be touched. It never made sense to me to try to > bundle new features / bug fixes with improvements to code quality. > > Making the code more reliable should be a goal in itself, rather than a > side effect of other work. > > Jon > > > > On Fri, May 9, 2025 at 10:31 AM Blake Eggleston <bl...@ultrablake.com> > wrote: > > > This seems like a cool feature that will be useful in future development > work, but not something we should be proactively refactoring the project to > make use of. > > On Fri, May 9, 2025, at 10:18 AM, Vivekanand Koya wrote: > > I would say that https://openjdk.org/jeps/394 (instanceOf) aims to > provide safer and less poisoning in the code by default. Instead of having > a production server halt/impaired due to a RuntimeException, instead it is > verified at compile time. If a new language feature makes code more robust > and addresses a hazardous, historical design choice, I believe it's time > has arrived. Curious to see what everyone thinks. > > Thanks, > Vivekanand K. > > On Fri, May 9, 2025 at 9:51 AM Josh McKenzie <jmcken...@apache.org> wrote: > > > I would like to refactor the codebase (Trunk 5+) to eliminate unsafe > explicit casting with instanceOf. > > We have a rich history of broad sweeping refactors dying on the rocks of > the community's aversion to instability and risk w/out a concrete outcome > we're trying to achieve. :) > > All of which is to say: do we have examples of instanceOf casting blowing > things up for users that would warrant going through the codebase to tidy > this up? Between src/java and test/unit and test/distributed we have around > 2,000 occurrences of this pattern. > > On Fri, May 9, 2025, at 10:14 AM, Vivekanand Koya wrote: > > Sounds great. I would like to refactor the codebase (Trunk 5+) to > eliminate unsafe explicit casting with instanceOf. > > Thanks, > Vivekanand > > On Fri, May 9, 2025, 5:19 AM Benedict Elliott Smith <bened...@apache.org> > wrote: > > Yep, that approach seems more than sufficient to me. No need for lots of > ceremony, but good to keep everyone in the decision loop. > > On 9 May 2025, at 13:10, Josh McKenzie <jmcken...@apache.org> wrote: > > I think it doesn’t cost us much to briefly discuss new language features > before using them. > > I had that thought as well but on balance my intuition was there were > enough new features that the volume of discussion to do that would be a > poor cost/benefit compared to the "lazy consensus, revert" approach. > > So if I actually do the work required to have an opinion ;): > > https://docs.oracle.com/en/java/javase/21/language/java-language-changes-release.html#GUID-6459681C-6881-45D8-B0DB-395D1BD6DB9B > > JDK21: > - Record Patterns > <https://docs.oracle.com/pls/topic/lookup?ctx=javase21&id=GUID-7623D3AD-4141-4914-A384-60C65BD0C010> > - Pattern Matching for switch Expressions and Statements > <https://docs.oracle.com/pls/topic/lookup?ctx=javase21&id=GUID-E69EEA63-E204-41B4-AA7F-D58B26A3B232> > - String Templates > <https://docs.oracle.com/pls/topic/lookup?ctx=javase21&id=GUID-78F545D3-CDD0-415C-9B4B-6EF361D184F5> > - Unnamed Patterns and Variables > <https://docs.oracle.com/pls/topic/lookup?ctx=javase21&id=GUID-D54E1CF1-BDFD-4B57-8A6E-5B4C87F4D58A> > - Unnamed Classes and Instance Main Methods > <https://docs.oracle.com/pls/topic/lookup?ctx=javase21&id=GUID-35544A22-61AB-4928-99BB-A9DD1CA062FF> > JDK17: > - Sealed Classes > <https://docs.oracle.com/pls/topic/lookup?ctx=javase17&id=GUID-0C709461-CC33-419A-82BF-61461336E65F> > JDK16: > - Pattern Matching for instanceof > <https://docs.oracle.com/pls/topic/lookup?ctx=javase16&id=GUID-843060B5-240C-4F47-A7B0-95C42E5B08A7> > JDK15: > - Text Blocks > <https://docs.oracle.com/pls/topic/lookup?ctx=javase15&id=GUID-221D06A2-FF54-4DB3-A6DA-179B8F76DB05> > JDK14: > - Switch Expressions > <https://docs.oracle.com/pls/topic/lookup?ctx=javase14&id=GUID-BA4F63E3-4823-43C6-A5F3-BAA4A2EF3ADC> > JDK11: > - Local Variable Type Inference > <https://docs.oracle.com/en/java/javase/21/language/local-variable-type-inference.html#GUID-D2C58FE6-1065-4B50-9326-57DD8EC358AC> > (test > only, not prod code is where we landed) > > Assuming we just lazily evaluate and deal with new features as people* > actually care about them* and seeing them add value, a simple "[DISCUSS] > I'm thinking about using new language feature X; any objection?" lazy > consensus that we then dumped onto a wiki article / code style page as > "stuff we're good to use" would probably be fine? > > > On Fri, May 9, 2025, at 7:58 AM, Benedict wrote: > > > I think it doesn’t cost us much to briefly discuss new language features > before using them. Lambdas, Streams and var all have problems - and even > with the guidance we publish some are still misused. > > The flow scoping improvement to instanceof seems obviously good though. > > > On 9 May 2025, at 12:30, Josh McKenzie <jmcken...@apache.org> wrote: > > > For new feature work on trunk, targeting the highest supported language > level featureset (jdk17 right now, jdk21 within the next couple of weeks) > makes sense to me. For bugfixing, targeting the oldest supported GA branch > and the highest language level that works there would allow maximum > flexibility with minimal re-implementation. > > If anyone has any misgivings with certain features (i.e. the debate around > usage of "var") they can bring it up on the dev ML and we can adjust, but > otherwise I'd prefer to see us have more modern evolving options on how > contributors engage rather than less. > > On Fri, May 9, 2025, at 1:56 AM, Vivekanand Koya wrote: > > Hello, > > I want to understand the community's thoughts on using newer features > (post JDK11) in upcoming releases in Cassandra. An example is flow scoping > instead of explicitly casting types with instanceOf: > https://openjdk.org/jeps/395. I want your thoughts on JDK requirements > for the main Cassandra repository, Accord, and Sidecar. > > Much appreciated. > Thanks, > Vivekanand K. > > > > > > >
