CASSANALYTICS-121: Assign data file start offset based on BTI index#173
CASSANALYTICS-121: Assign data file start offset based on BTI index#173lukasz-antoniak wants to merge 1 commit intoapache:trunkfrom
Conversation
lukasz-antoniak
force-pushed
the
CASSANALYTICS-121
branch
from
5aee749 to
18af425
Compare
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
Thanks for the patch!
Please also update CHANGES.txt
| if (position.size() == 1) | ||
| { | ||
| offset.set(position.get(0).lowerPosition); | ||
| } |
There was a problem hiding this comment.
- nit: rename to
positions? - When the positions size is not 1, it is simply ignored. It is expected? If so, please make a comment
| try | ||
| { | ||
| // Loading data file is required by BtiTableReader. | ||
| // Migrate to org.apache.cassandra.io.sstable.format.SSTableReader#getApproximatePositionsForRanges |
There was a problem hiding this comment.
I cannot find the method in Cassandra trunk a9ee34b62d977893380b0b753c25b2b0aa68fa11. Is it from an unmerged patch in Cassandra?
How about filing a JIRA (in Analytics) and comment here to not forget?
There was a problem hiding this comment.
It is available in DS distribution. Registered CASSANALYTICS-123 and removed the comment.
| Token tokenStart = TokenUtils.bigIntegerToToken(metadata.partitioner, tokenRange.lowerEndpoint()); | ||
| Token tokenEnd = TokenUtils.bigIntegerToToken(metadata.partitioner, tokenRange.upperEndpoint()); | ||
| Range<Token> range = new Range<>(tokenStart, tokenEnd); |
There was a problem hiding this comment.
those lines are unlikely to throw, but if it happens, the btiTableReader is not released. Can we ensure the reader is released?
| { | ||
| // o.a.c.io.util.SimpleChunkReader flips the buffer, so position should be set to the end. | ||
| dst.position(read); | ||
| dst.position(Math.min(read, dst.limit())); |
There was a problem hiding this comment.
When read can be greater than dst.limit()? A bit concerned the guard masks some true bug, if Math.min is necessary.
There was a problem hiding this comment.
If I think about it again, you might be right. Wanted to be more defensive coding, but this should not happen. This is a NIT outside scope of this PR. I have observed it only when testing with DS Cassandra, not OSS. Reverting.
| throw new UnsupportedOperationException("Unexpected token type: " + token.getClass().getName()); | ||
| } | ||
|
|
||
| public static Token bigIntegerToToken(final IPartitioner partitioner, final BigInteger token) |
There was a problem hiding this comment.
nit: remove final.
The method seem to be unused?
There was a problem hiding this comment.
IntelliJ does not see it at first, because the class is present in cassandra-four-zero-types module. cassandra-five-zero-types is based on cassandra-four-zero-types, but class body does not change. Executing a Gradle build fixes the problem.
lukasz-antoniak
force-pushed
the
CASSANALYTICS-121
branch
from
18af425 to
eca59fa
Compare
Patched by Lukasz Antoniak; Reviewed by Yifan Cai for CASSANALYTICS-121
lukasz-antoniak
force-pushed
the
CASSANALYTICS-121
branch
from
eca59fa to
5a8b699
Compare
|
@yifan-c, thank you for quick review. PR ready for a second round. CI in progress. |
2 participants
Fixes CASSANALYTICS-121.