Skip to content

Conversation

adelapena
Copy link

Don't purge rows in the replicas when we are using bytes-sized limits. Otherwise, the coordinator can get rows with a size lower than the requested page size and think that there is no more data.

@adelapena adelapena self-assigned this Sep 22, 2025
Copy link

Checklist before you submit for review

  • This PR adheres to the Definition of Done
  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

@adelapena adelapena force-pushed the CNDB-15435-main-dont-purge branch from f2925cb to 5f39850 Compare September 23, 2025 10:25
@adelapena adelapena changed the title CND-15435: Don't purge rows in the replicas with bytes-sized paging CNDB-15435: Don't purge rows in the replicas with bytes-sized paging Sep 23, 2025
Copy link

@cassci-bot
Copy link

✔️ Build ds-cassandra-pr-gate/PR-2009 approved by Butler


Approved by Butler
See build details here

Comment on lines +452 to +455
// We only purge rows before sending them to the coordinator if the limit isn't in bytes.
// Otherwise, the replica can send rows with a size lower than the requested page size,
// making the coordinator think that there is no more data and wrongly end paging.
// See CNDB-15435 for further details.

Choose a reason for hiding this comment

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

Does it affect performance?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed it does, both PRs reduce protection against tombstone overwhelming.

Copy link

@pkolaczk pkolaczk left a comment

Choose a reason for hiding this comment

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

Approving because correctness > performance.
However, I don't like the fact it introduces yet another special case (purging works in one case, but not in another) and I agree that a flag-based approach (passing information to the coordinator) would be less fragile.

@adelapena
Copy link
Author

Closed in favor of #2010

@adelapena adelapena closed this Sep 23, 2025
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