blambov commented on code in PR #4245:
URL: https://github.com/apache/cassandra/pull/4245#discussion_r2231350425
##########
src/java/org/apache/cassandra/db/memtable/TrieMemtable.java:
##########
@@ -360,8 +360,8 @@ public FlushablePartitionSet<MemtablePartition>
getFlushSet(PartitionPosition fr
{
boolean allPositionsToFlush = from == null && to == null
|| from != null && to != null
- && from.isMinimum()
- && to.isMaximum();
+ && from.isMinimum() && from.kind() ==
PartitionPosition.Kind.MIN_BOUND
Review Comment:
The minimum token is something reserved for boundaries (i.e. the computed
hash is adjusted to avoid it) thus the `MIN_BOUND` check here is unnecessary.
AFAIAA other uses of `isMinimum` don't check this, and I believe we also use
minimum with `MAX_BOUND` in some cases, which will fail this check when the
intention is to fully cover the lower side of the ring.
##########
src/java/org/apache/cassandra/db/memtable/TrieMemtable.java:
##########
@@ -382,25 +383,32 @@ public FlushablePartitionSet<MemtablePartition>
getFlushSet(PartitionPosition fr
int toShardFull;
int toShardPartial;
- if (from == null || from.isMinimum())
+ if (from == null || from.isMinimum() && from.kind() ==
PartitionPosition.Kind.MIN_BOUND)
Review Comment:
... also applicable here.
##########
src/java/org/apache/cassandra/dht/Token.java:
##########
@@ -425,6 +430,11 @@ public boolean isMinimum()
return getToken().isMinimum();
}
+ public boolean isMaximum()
+ {
+ return getToken().isMaximum();
Review Comment:
As explained above, we do need `&& !isMinimumBound` here.
##########
src/java/org/apache/cassandra/db/memtable/TrieMemtable.java:
##########
@@ -382,25 +383,32 @@ public FlushablePartitionSet<MemtablePartition>
getFlushSet(PartitionPosition fr
int toShardFull;
int toShardPartial;
- if (from == null || from.isMinimum())
+ if (from == null || from.isMinimum() && from.kind() ==
PartitionPosition.Kind.MIN_BOUND)
fromShardFull = fromShardPartial = 0;
else
{
fromShardPartial = boundaries.getShardForToken(from.getToken());
- Token fromStartBoundaryToken =
boundaries.getShardStartBoundary(fromShardPartial);
- if (fromStartBoundaryToken != null &&
from.equals(fromStartBoundaryToken.maxKeyBound()))
+ // not symmetric to "to" logic because the start part of shard
ranges is exclusive
+ Token fromEndBoundaryToken =
boundaries.getShardEndBoundary(fromShardPartial);
+ if (fromEndBoundaryToken != null &&
from.equals(fromEndBoundaryToken.maxKeyBound()))
+ {
+ fromShardPartial++;
fromShardFull = fromShardPartial;
+ }
else
fromShardFull = fromShardPartial + 1;
}
- if (to == null || to.isMaximum())
+ if (to == null || to.isMaximum() && to.kind() ==
PartitionPosition.Kind.MAX_BOUND)
Review Comment:
... and here
##########
src/java/org/apache/cassandra/db/memtable/TrieMemtable.java:
##########
@@ -360,8 +360,8 @@ public FlushablePartitionSet<MemtablePartition>
getFlushSet(PartitionPosition fr
{
boolean allPositionsToFlush = from == null && to == null
|| from != null && to != null
- && from.isMinimum()
- && to.isMaximum();
+ && from.isMinimum() && from.kind() ==
PartitionPosition.Kind.MIN_BOUND
+ && to.isMaximum() && to.kind() ==
PartitionPosition.Kind.MAX_BOUND;
Review Comment:
To clarify (or add to the confusion), the maximum is sometimes out of scope
(`RandomPartitioner`) and other times isn't (`Murmur3Partitioner`). The
`MAX_BOUND` check is necessary for the maximum, but it should be part of the
`isMaximum` call rather than done here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
