CASSANDRA-21671 Fix compressed scan read-ahead buffer Block leak and chunk-cache pollution by scans - #5168
CASSANDRA-21671 Fix compressed scan read-ahead buffer Block leak and chunk-cache pollution by scans#5168maedhroz wants to merge 5 commits into
Conversation
…ution by scans A shared thread-local read-ahead buffer allowed concurrent scans of one SSTable to collide and leak. Each compressed scan reader now owns a per-scan buffer that it frees on close. Scans also polluted the chunk cache with one-shot chunks that evicted hot data. The new ReadPattern concept allows us to route single-partition and partition-range reads through the cache while scans bypass it and read ahead through their own buffer. patch by Jon Haddad; reviewed by Caleb Rackliffe and Sam Lightfoot for CASSANDRA-21671 Co-authored-by: Jon Haddad <jon@jonhaddad.com> Co-authored-by: Caleb Rackliffe <calebrackliffe@gmail.com> Assisted-by: Claude Code:claude-opus-4-8
| } | ||
|
|
||
| public ThreadLocalReadAheadBuffer(ChannelProxy channel, int bufferSize, BufferType bufferType) | ||
| public ReadAheadBuffer(ChannelProxy channel, Supplier<ByteBuffer> bufferSupplier) |
There was a problem hiding this comment.
nit: ctor not needed as the Direct I/O subclass is introduced in v6.
| /** Single-partition reads use the chunk cache but do not read ahead. */ | ||
| ROW_READ(true, false), | ||
|
|
||
| /** Range queries walking partitions in order use the chunk cache and can read ahead only if the cache is disabled. */ |
There was a problem hiding this comment.
nit: extra space after 'order'
| /** Single-partition reads use the chunk cache but do not read ahead. */ | ||
| ROW_READ(true, false), | ||
|
|
||
| /** Range queries walking partitions in order use the chunk cache and can read ahead only if the cache is disabled. */ |
There was a problem hiding this comment.
NIT
| /** Range queries walking partitions in order use the chunk cache and can read ahead only if the cache is disabled. */ | |
| /** Range queries walking partitions in order use the chunk cache and can read ahead only if the cache is disabled. */ |
| private final ReadAheadBuffer readAheadBuffer; | ||
|
|
||
| private ScanCompressedReader(ChannelProxy channel, CompressionMetadata metadata, int readAheadBufferSize) | ||
| private ScanCompressedReader(ChannelProxy channel, ThreadLocalByteBufferHolder bufferHolder, |
There was a problem hiding this comment.
not sure I understand the signature change here, doesn't seem we gain anything. I would keep as it was before. It is functionally strictly the same.
| ByteBuffer buffer = null; | ||
| int index = -1; | ||
| } | ||
| protected final ChannelProxy channel; |
There was a problem hiding this comment.
can this be private ?
| protected final ChannelProxy channel; | |
| private final ChannelProxy channel; |
| protected final ChannelProxy channel; | ||
|
|
||
| private final ChannelProxy channel; | ||
| private final Supplier<ByteBuffer> bufferSupplier; |
There was a problem hiding this comment.
this should be BufferType, we don't have any use case for supplier here
| private final Supplier<ByteBuffer> bufferSupplier; | |
| private final BufferType bufferType; |
| } | ||
|
|
||
| public ThreadLocalReadAheadBuffer(ChannelProxy channel, int bufferSize, BufferType bufferType) | ||
| public ReadAheadBuffer(ChannelProxy channel, Supplier<ByteBuffer> bufferSupplier) |
| doReads(f, metadata1, length, false); | ||
| int raReads = reads.getAndSet(0); |
There was a problem hiding this comment.
do you mean this should be true here instead? This is a RA read?
| doReads(f, metadata1, length, false); | |
| int raReads = reads.getAndSet(0); | |
| doReads(f, metadata1, length, true); | |
| int raReads = reads.getAndSet(0); |
|
|
||
| doReads(f, metadata2, length, true); | ||
| int scanReads = reads.getAndSet(0); |
There was a problem hiding this comment.
similarly shouldn't this be false here?
| { | ||
| return this; | ||
| // Bypass the cache for patterns that read data once, so they don't evict hot data. | ||
| return pattern.usesCache() ? this : source.instantiateRebufferer(pattern); |
There was a problem hiding this comment.
when read ahead is disabled (i.e. compressed_read_ahead_buffer_size = 0) should we still use the cache?
|
|
||
| block.index = -1; | ||
| if (block.buffer == null) | ||
| if (buffer == null) |
There was a problem hiding this comment.
deallocate is always true , even in tests. Should we just remove this param?
There was a problem hiding this comment.
Looking at other places where we call org.apache.cassandra.io.sstable.format.SSTableReader#openDataReader() -> createReader() it looks like we do it in ForwardingSSTableReader, StorageAttachedIndexBuilder, SASIIndexBuilder, SortedTableScrubber, SortedTableVerifier. Might be worth thinking about those too.
4 participants
This patch is essentially a 5.0 version of #5152, with a few, mostly superficial simplifications. The core is the same:
1.) We're no longer using thread-locals, which allows us to avoid an entire class of bookkeeping errors.
2.) Scans during compaction bypass the chunk cache to avoid needlessly evicting hot data.