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.
