-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-15393: fix ShardManager#coveringRange to decrement left instead of incrementing right #2003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Checklist before you submit for review
|
4060fd9
to
15c7728
Compare
@Override | ||
public Token prevValidToken() | ||
{ | ||
return isMinimum() ? new LongToken(Long.MAX_VALUE) : new LongToken(token - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would let wraparound take care of the min->max transition as in nextValidToken
.
Note: Any decorated key would have a non-minimum token, so wraparound here could not happen for sstable start keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 26932c4
@Override | ||
public Token prevValidToken() | ||
{ | ||
BigInteger prev = this.isMinimum() || token.compareTo(ZERO) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. MINIMUM
is not a real ring position, but it makes better sense for what we are trying to do with this method as an adjustment for a ZERO
token for an SSTable's first key. There is some risk that wrapping around will cause trouble in some function that is only written to work with non-wraparound ranges: these normally accept minimum as the upper bound, but not maximum on the lower side.
I would change this to go from ZERO
to MINIMUM
and either not touch MINIMUM
or wrap it around. I know this means MAXIMUM.nextValidToken().prevValidToken() != MAXIMUM
, but that's something I'd more easily accept (especially if we add a comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. fixed in 26932c4
…of incrementing right, because Range is left exclusive * Added Token#prevValidToken to return previous possible tokens for 2 hashing partitioner
26932c4
to
2e876b7
Compare
2e876b7
to
9e2e259
Compare
…f checking PartitionPosition
I have pushed e71ac0f to check if token is minimal instead of checking if partition-position is minimal. @blambov wdyt? |
|
❌ Build ds-cassandra-pr-gate/PR-2003 rejected by Butler6 regressions found Found 6 new test failuresNo known test failures found |
What is the issue
CNDB-15393 -
ShardManager#coveringRange
returns a range missing left position and theShardManager#rangeSpanned
for (min, min] is not 1.What does this PR fix and why was it fixed
Create Range with (left-1, right] to make sure both left and right are included. And make sure range span is 1 for (min, min]