Skip to content

CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader - #234

Open
mansikhara wants to merge 3 commits into
apache:trunkfrom
mansikhara:CASSANALYTICS-180
Open

CASSANALYTICS-180: TokenPartitioner fails to detect range gap in reader#234
mansikhara wants to merge 3 commits into
apache:trunkfrom
mansikhara:CASSANALYTICS-180

Conversation

@mansikhara

Copy link
Copy Markdown

No description provided.

@jyothsnakonisa jyothsnakonisa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Please add Changes.txt entry for this patch


RangeSet<BigInteger> uncovered = TreeRangeSet.create();
uncovered.add(fullRange);
coveringRanges.forEach(uncovered::remove);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
coveringRanges.forEach(uncovered::remove);
uncovered.removeAll(coveringRanges);

You could use removeAll instead of removing each entry in for loop

@yifan-c

yifan-c commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants