Skip to content

NativeAPI: Update getRangeStream read conflict ranges continuously as results are returned - #14323

Open
Ronitsabhaya75 wants to merge 2 commits into
apple:mainfrom
Ronitsabhaya75:fix/nativeapi-conflict
Open

Ronitsabhaya75 wants to merge 2 commits into
apple:mainfrom
Ronitsabhaya75:fix/nativeapi-conflict

Conversation

@Ronitsabhaya75

Copy link
Copy Markdown
Contributor

Problem

getRangeStream was registering the entire requested key range [min(b, begin), max(e, end)] as a read conflict range upfront before reading any data. This resulted in false conflict aborts if the client only read part of the stream and committed, if the range was empty/inverted (b >= e), or if an error occurred before reaching later shards.

Solution

  • Track streaming conflict ranges in TransactionState and extend the range incrementally in getRangeStreamImpl as each chunk is yielded to the caller.
  • Append accumulated stream conflict ranges into tr.transaction.read_conflict_ranges when preparing commitMutations().
  • Return immediately on empty ranges (b >= e) without adding a conflict range.

Testing

  • Validated lifecycle against NativeAPI conflict range handling and TransactionState reset/commit paths.

@saintstack saintstack 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.

LG. Local LLM found an issue:

An empty resolved range (b >= e) now records no read conflict range. The old code recorded [min(b, begin.getKey()), max(e, end.getKey())) before this check. Resolving the selectors already read keys between begin.getKey() and b and between e and end.getKey(). A concurrent insert there could make the range non-empty, and the transaction would no longer conflict. Non-stream getRange still records a range for an empty result (getRangeFinished). The early return in Transaction::getRangeStream, before any resolution, is fine because nothing has been read yet. Fix: when !snapshot, add [rangeBegin, rangeEnd) before the end_of_stream return in ::getRangeStream.

@saintstack saintstack 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.

Here are another few you might to look at:

  • Reverse branches: the reverse logic at NativeAPI.cpp:3430 and :3448 is wrong, but unreachable. Transaction::getRangeStream asserts !reverse.
  • readConflictRanges() lifetime: it uses a shallow push_back of arena-backed ranges (NativeAPI.h:493-500). It could dangle after reset, but nothing in the RYW layer calls getRangeStream.
  • Unbounded range growth: one range is added per chunk, never merged, and added after the transaction_too_large size check. This is a scalability regression rather than a correctness bug.
  • Abandoned streams: an abandoned stream with an end selector of offset <= 0 skips the end-selector region.
  • Tests and style: the PR adds no tests, and the getRangeStreamImpl call at line 3528 is over 120 columns.

@Ronitsabhaya75

Copy link
Copy Markdown
Contributor Author

@saintstack thank you for your review I've update pr according to your review

This branch has not been deployed

No deployments
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.

2 participants