Hi Folks, A bit delayed but I have the backport for 20092 ready. The branch can be found here: https://github.com/apache/cassandra/compare/cassandra-5.0...jrwest:cassandra:jwest/20092-5.0-backport. I've run tests and all looked good. I plan to do one more run post a recent rebase. Links are in CASSANDRA-20092. Before merging I wanted to sunshine and get community input on the following:
* I've also included CASSANDRA-20396 in the backport. This looks to be a fix on top of CASSANDRA-20092 that we would want in the backport. I plan to leave these as two separate commits on cassandra-5.0, not one merged commit. If you feel differently please let me know. * CHANGES.txt: i've included both patches in 5.0.5 section of CHANGES.txt. They also appear under 5.1 in trunk. Since trunk isn't released I don't see any issue here but if folks have strong opinions on this lmk. I don't and can make any requested changes. * Along the way I discovered CASSANDRA-20538, a minor 1-line change needed to actually take advantage of 20092+15452 when BTI is enabled. I have included the patch for this in the backport as its largely a ninja fix. If folks feel strongly about addressing both 5.0 and trunk with 20538 instead of one coming via backport and one coming via the JIRA I can remove that commit from my backport (I am also not sure the best way to commit this in the backport, so I sort of lean to immediately addressing it in 20538. its largely moot since both would be included in the release either way). * For the backport I had to adjust one test ( https://github.com/apache/cassandra/commit/0039ebf915b66a88234325a74ef6edd18bde6da0). >From what I can tell this is just because the compression code is less likely to find an aligned key on 5.0 (I haven't dug too much into why now that the test reliably passes with this minor tweak). Plan to squash this into the backport commit for CASSANDRA-20092 unless folks object. Jordan On Wed, Feb 19, 2025 at 1:35 PM Ariel Weisberg <ar...@weisberg.ws> wrote: > Whoops, 5 months ago, not six months ago. Much more reasonable to be > making this kind of fix. > > On Wed, Feb 19, 2025, at 3:56 PM, Ariel Weisberg wrote: > > Hi, > > This does not constitute a review, but I looked at both of them to > convince myself how they go about solving their respective problems is a > good idea. I am weakly +1. The risk reward is there, but 13 months since > 5.0 was released feels a little late to trying to improve node density > instead of just saying it is what it is. > > The +1 stems from the fact that if you really operate this at scale and > you are committed to node density then it's less of a nice to have and more > of a "some cluster falls over periodically now". > > Ariel > > On Fri, Feb 14, 2025, at 5:23 PM, Jordan West wrote: > > Thanks for the write up Mick. I think its is a great evaluation of 15452. > A few notes below: > > * CI links for 15452 might be burried and I may need to link the most > recent run (I’ve been using CircleCI since it’s what I’m familiar with — > happy to have runs on ASf hardware as well). > > * 15452 is configurable by YAML property as you mentioned. I’m traveling > but I’m 99% sure I made it a hot property as well (personally I believe you > shouldn’t have to restart a node to disable something like this, so if I > forgot to make it hot I will before merge). I’m traveling without my laptop > so I will double check next week (if someone has time to before then it’s > much appreciated) > > * I think “safe-ish” is a fair description of 15452. The new code is > fairly isolated but it’s in a very critical path with several call paths > leading to it. I think the testing we’ve done (both manual and automated) > as well as finding and fixing several bugs with those tests is what gives > me the confidence. And as you mentioned we have the fail safe of turning it > off otherwise. > > Jordan > > On Fri, Feb 14, 2025 at 12:42 Mick Semb Wever <m...@apache.org> wrote: > > Solid write up Jon! > > Hoping the committers and PMC members are keeping in mind this (very) > recent thread: > https://lists.apache.org/thread/h38g6q9d8h1q92h6qzs5tqdxpn2vmnyy > > This thread needs to also be about evaluating the risk these commits > are to a patch version. > I'm +1 and here's my thinking over it… > > Are the performance benefits clear ? > Yes (thank you Jon for doing a solid job at demonstrating just how > awesome this will be). > > Do we want this in 5.0 ? > That's the dumb question :) of course. > > Does it fix a bug or a regression, or is an operational improvement ? > Kinda, Branimir did mention a performance regression when the chunk > cache was disabled. > > How are the patches with unforeseen risks ? > These patches are medium sized and low-level, though isolated to (and > creating a better isolation of) the compaction (scan) reader. 15452 > looks safe-ish. I can't speak to 20092, it would be nice to hear > Branimir and Sylvain chime in. Are we confident that if any bugs do > arise in user's 5.0 production deployments we will be quick to provide > fixes and releases? I was thinking it's worth putting in a system > property that can disable the scan reader, so if anything happens > folks can just restart the node with the system property to return to > pre-patch behaviour, but that's already there! :) > > How well tested have they been ? > We have one (or a few) reports of it running in production, that's > super positive. Reports of lots of manual testing, super positive > too. But there's no CI results available for either patch. (Even the > one committed to trunk doesn't have pre-commit CI results available.) > I think CI results attached to the ticket are a must – I'm working on > adding them. > > > >
