[CASSANDRA-21637][trunk] Avoid double chunk read for BTI small partitions during a select query execution - #5098
[CASSANDRA-21637][trunk] Avoid double chunk read for BTI small partitions during a select query execution#5098netudima wants to merge 2 commits into
Conversation
| * iterator has to start. {@link #dataInput} lets that reader be reused instead of closed, so the iterator does | ||
| * not have to open the same position again; on a compressed table that would decompress and checksum the same | ||
| * chunk twice. It is null whenever there is nothing to reuse, and when it is not null the caller owns it and | ||
| * must close it. |
There was a problem hiding this comment.
Could you also explain why the generic getPosition path does not need this?
There was a problem hiding this comment.
Yep, I've extended the comment
| static final ExactPosition NOT_FOUND = new ExactPosition(null, null); | ||
|
|
||
| final TrieIndexEntry entry; | ||
| final FileDataInput dataInput; |
There was a problem hiding this comment.
I'm happy to accept this, but it does feel a bit risky to return an opened reader here.
Have you considered an option to carry a "query workset" of a kind with an operation, something that is closeable, opened in a try block by the caller, or by an upper-level task such as a query, and passed as an argument to this and other methods so that they can reuse readers and leave the closing to that workset object?
There was a problem hiding this comment.
yes, I was thinking about it but it looks for me a way too complicated: The reader is closed only once we've finished reading, so I have to propagate this context through many calls up to a high level, like:
org.apache.cassandra.db.rows.UnfilteredSource#rowIterator (which is invoked in several places, so it means I have to do try logic in all such places to avoid potential leaks).
org.apache.cassandra.db.SinglePartitionReadCommand#queryMemtableAndDisk (again many places this method is invoked by, same story)
So, for me it looks like there are more risks to make a mistake in such large refactoring vs the current limited change with to attaching to SSTableIterator lifecycle (what we did in some cases already)..
2 participants
CASSANDRA-21637