You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "mansikhara (via GitHub)" <gi...@apache.org> on 2026/08/19 21:01:08 UTC
[PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
mansikhara opened a new pull request, #234:
URL: https://github.com/apache/cassandra-analytics/pull/234
(no comment)
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "mansikhara (via GitHub)" <gi...@apache.org>.
mansikhara commented on code in PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234#discussion_r3848336063
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/RangeUtils.java:
##########
@@ -70,6 +74,54 @@ public static boolean isOpenClosedRange(Range<?> range)
return range.lowerBoundType() == BoundType.OPEN && range.upperBoundType() == BoundType.CLOSED;
}
+ /**
+ * Finds the sub-ranges of the whole token ring that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the ring coverage. An empty result means the ring is covered in its entirety.
+ * <p>
+ * The ring is expressed as {@code (minToken, maxToken]}, matching the open-closed notation used for token
+ * ranges everywhere else. Expressing it as {@code [minToken, maxToken]} instead would report minToken as a
+ * spurious single-token gap, because open-closed ranges never cover their own lower endpoint.
+ *
+ * @param partitioner the partitioner whose ring bounds are expected to be covered
+ * @param coveringRanges the ranges expected to cover the ring; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of the ring, in ascending order
+ */
+ public static List<Range<BigInteger>> findUncoveredRingRanges(Partitioner partitioner,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ return findUncoveredRanges(Range.openClosed(partitioner.minToken(), partitioner.maxToken()), coveringRanges);
+ }
+
+ /**
+ * Finds the sub-ranges of {@code fullRange} that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the coverage. An empty result means {@code coveringRanges} covers {@code fullRange} in its entirety.
+ * <p>
+ * Both the input and the output honor Guava's bound types, so {@code fullRange} must be expressed using the
+ * same notation as the covering ranges. That makes the method easy to misuse, which is why it is not exposed
+ * beyond this class: callers go through {@link #findUncoveredRingRanges}, which owns the notation so that the
+ * bound types are not decided at each call site. Empty ranges are never reported, as a range that contains no
+ * token cannot be a gap.
+ *
+ * @param fullRange the non-empty range expected to be fully covered
+ * @param coveringRanges the ranges expected to cover {@code fullRange}; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of {@code fullRange}, in ascending order
+ */
+ @VisibleForTesting
+ static List<Range<BigInteger>> findUncoveredRanges(Range<BigInteger> fullRange,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ // An empty fullRange has nothing to cover, so every input would trivially look fully covered.
+ // Reject it rather than report a false "no gaps".
+ Preconditions.checkArgument(!fullRange.isEmpty(), "fullRange must not be empty");
+
+ RangeSet<BigInteger> uncovered = TreeRangeSet.create();
+ uncovered.add(fullRange);
+ coveringRanges.forEach(uncovered::remove);
Review Comment:
done!
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234#issuecomment-5460838467
Sorry for the belated review.
First of all, thanks for adding more test cases.
I think the fix can be much smaller than what's here. The actual bug is just that `.filter(Range::isEmpty)` in `validateCompleteRangeCoverage()` is a no-op: Guava's TreeRangeSet never materializes empty ranges in `asRanges()`, so that filter can never keep anything, and the missing-range check silently always passes.
The minimal and correct fix is:
- Drop the `.filter(Range::isEmpty)` filter
- Change `Range.closed(minToken, maxToken)` to `Range.openClosed(minToken, maxToken)`, since sub-ranges everywhere else are open-closed and closed would falsely flag minToken as a gap on an otherwise healthy ring
Would you mind trimming the patch down to that?
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234#discussion_r3901342348
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/TokenPartitioner.java:
##########
Review Comment:
We should really consolidate the two `TokenPartitioner`s, some day... Not in scope for this patch, I am just mentioning.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "mansikhara (via GitHub)" <gi...@apache.org>.
mansikhara commented on code in PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234#discussion_r3848336063
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/RangeUtils.java:
##########
@@ -70,6 +74,54 @@ public static boolean isOpenClosedRange(Range<?> range)
return range.lowerBoundType() == BoundType.OPEN && range.upperBoundType() == BoundType.CLOSED;
}
+ /**
+ * Finds the sub-ranges of the whole token ring that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the ring coverage. An empty result means the ring is covered in its entirety.
+ * <p>
+ * The ring is expressed as {@code (minToken, maxToken]}, matching the open-closed notation used for token
+ * ranges everywhere else. Expressing it as {@code [minToken, maxToken]} instead would report minToken as a
+ * spurious single-token gap, because open-closed ranges never cover their own lower endpoint.
+ *
+ * @param partitioner the partitioner whose ring bounds are expected to be covered
+ * @param coveringRanges the ranges expected to cover the ring; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of the ring, in ascending order
+ */
+ public static List<Range<BigInteger>> findUncoveredRingRanges(Partitioner partitioner,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ return findUncoveredRanges(Range.openClosed(partitioner.minToken(), partitioner.maxToken()), coveringRanges);
+ }
+
+ /**
+ * Finds the sub-ranges of {@code fullRange} that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the coverage. An empty result means {@code coveringRanges} covers {@code fullRange} in its entirety.
+ * <p>
+ * Both the input and the output honor Guava's bound types, so {@code fullRange} must be expressed using the
+ * same notation as the covering ranges. That makes the method easy to misuse, which is why it is not exposed
+ * beyond this class: callers go through {@link #findUncoveredRingRanges}, which owns the notation so that the
+ * bound types are not decided at each call site. Empty ranges are never reported, as a range that contains no
+ * token cannot be a gap.
+ *
+ * @param fullRange the non-empty range expected to be fully covered
+ * @param coveringRanges the ranges expected to cover {@code fullRange}; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of {@code fullRange}, in ascending order
+ */
+ @VisibleForTesting
+ static List<Range<BigInteger>> findUncoveredRanges(Range<BigInteger> fullRange,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ // An empty fullRange has nothing to cover, so every input would trivially look fully covered.
+ // Reject it rather than report a false "no gaps".
+ Preconditions.checkArgument(!fullRange.isEmpty(), "fullRange must not be empty");
+
+ RangeSet<BigInteger> uncovered = TreeRangeSet.create();
+ uncovered.add(fullRange);
+ coveringRanges.forEach(uncovered::remove);
Review Comment:
done!
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader [cassandra-analytics]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #234:
URL: https://github.com/apache/cassandra-analytics/pull/234#discussion_r3848216958
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/RangeUtils.java:
##########
@@ -70,6 +74,54 @@ public static boolean isOpenClosedRange(Range<?> range)
return range.lowerBoundType() == BoundType.OPEN && range.upperBoundType() == BoundType.CLOSED;
}
+ /**
+ * Finds the sub-ranges of the whole token ring that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the ring coverage. An empty result means the ring is covered in its entirety.
+ * <p>
+ * The ring is expressed as {@code (minToken, maxToken]}, matching the open-closed notation used for token
+ * ranges everywhere else. Expressing it as {@code [minToken, maxToken]} instead would report minToken as a
+ * spurious single-token gap, because open-closed ranges never cover their own lower endpoint.
+ *
+ * @param partitioner the partitioner whose ring bounds are expected to be covered
+ * @param coveringRanges the ranges expected to cover the ring; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of the ring, in ascending order
+ */
+ public static List<Range<BigInteger>> findUncoveredRingRanges(Partitioner partitioner,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ return findUncoveredRanges(Range.openClosed(partitioner.minToken(), partitioner.maxToken()), coveringRanges);
+ }
+
+ /**
+ * Finds the sub-ranges of {@code fullRange} that none of the {@code coveringRanges} covers, i.e. the gaps in
+ * the coverage. An empty result means {@code coveringRanges} covers {@code fullRange} in its entirety.
+ * <p>
+ * Both the input and the output honor Guava's bound types, so {@code fullRange} must be expressed using the
+ * same notation as the covering ranges. That makes the method easy to misuse, which is why it is not exposed
+ * beyond this class: callers go through {@link #findUncoveredRingRanges}, which owns the notation so that the
+ * bound types are not decided at each call site. Empty ranges are never reported, as a range that contains no
+ * token cannot be a gap.
+ *
+ * @param fullRange the non-empty range expected to be fully covered
+ * @param coveringRanges the ranges expected to cover {@code fullRange}; they may overlap and need not be sorted
+ * @return the uncovered sub-ranges of {@code fullRange}, in ascending order
+ */
+ @VisibleForTesting
+ static List<Range<BigInteger>> findUncoveredRanges(Range<BigInteger> fullRange,
+ Collection<Range<BigInteger>> coveringRanges)
+ {
+ // An empty fullRange has nothing to cover, so every input would trivially look fully covered.
+ // Reject it rather than report a false "no gaps".
+ Preconditions.checkArgument(!fullRange.isEmpty(), "fullRange must not be empty");
+
+ RangeSet<BigInteger> uncovered = TreeRangeSet.create();
+ uncovered.add(fullRange);
+ coveringRanges.forEach(uncovered::remove);
Review Comment:
```suggestion
uncovered.removeAll(coveringRanges);
```
You could use removeAll instead of removing each entry in for loop
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org