Skip to content

Add optional traversal limit for commit offset lookup#4007

Open
NssGourav wants to merge 2 commits intoapache:mainfrom
NssGourav:fix/commit-offset-traversal-limit
Open

Add optional traversal limit for commit offset lookup#4007
NssGourav wants to merge 2 commits intoapache:mainfrom
NssGourav:fix/commit-offset-traversal-limit

Conversation

@NssGourav
Copy link

@NssGourav NssGourav commented Mar 17, 2026

Summary

  • Add a new polaris.persistence.commit-offset-lookup-max-traversal parameter (default 0 = unlimited) to optionally bound commit history traversal while resolving offsets.
  • Enforce the limit in CommitsImpl.commitLog() and CommitsImpl.commitLogReversed() during offset lookup, failing fast with a clear exception when exceeded.
  • Add tests covering the fail-fast behavior for both variants.

Rationale

CommitsImpl currently walks commit history unbounded when resolving offsets. In pathological cases (very large histories or missing/far-back offsets) this can lead to unbounded work.

The safeguard is opt-in (default unlimited) to avoid breaking background/maintenance flows that may legitimately traverse the full log.

Test plan

  • ./gradlew :persistence:nosql:persistence-impl:test

Fixes #4000

Introduce a PersistenceParams knob to bound offset resolution in CommitsImpl and fail fast when exceeded, with coverage verifying the safeguard for both commitLog variants.

Made-with: Cursor
@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 17, 2026
@NssGourav NssGourav closed this Mar 17, 2026
@NssGourav NssGourav deleted the fix/commit-offset-traversal-limit branch March 17, 2026 08:44
@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Mar 17, 2026
@NssGourav NssGourav restored the fix/commit-offset-traversal-limit branch March 17, 2026 08:46
@NssGourav NssGourav reopened this Mar 17, 2026
@github-project-automation github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Mar 17, 2026
@NssGourav NssGourav marked this pull request as draft March 17, 2026 08:47
@NssGourav NssGourav marked this pull request as ready for review March 17, 2026 08:58
@Subham-KRLX
Copy link
Contributor

Subham-KRLX commented Mar 17, 2026

A few things worth addressing like the limit is still inside CommitsImpl, but @snazy mentioned the TODO was meant for the call sites (REST/service layer), not the implementation itself worth clarifying with him before going further. There's also a counter bug where commitLog and commitLogReversed count traversals differently since traversed starts at 0 in one and 1 in the other, so the same maxTraversal value behaves inconsistently. On the exception side, IllegalStateException feels off for an expected operational limit a dedicated exception would be easier to catch specifically. For the tests, the .hasNext() in the commitLog test is misleading since the exception fires during the commitLog() call itself, not lazily, and there's also no test for the success case where the offset is found within the limit. Overall the approach is reasonable, just needs a quick sync with the maintainer on the architectural question first.

Use a dedicated exception for limit exceedance, make traversal counting consistent across log variants, and expand tests to cover both fail-fast and success paths.

Made-with: Cursor
@NssGourav
Copy link
Author

Thanks agreed on the feedback.

I understand the TODOs may be intended for call-site guardrails; I implemented an opt-in safeguard in CommitsImpl (commit-offset-lookup-max-traversal, default 0 = unlimited) to avoid impacting maintenance flows unless configured.

Fixed traversal counting so commitLog and commitLogReversed behave consistently.
Replaced IllegalStateException with CommitOffsetTraversalLimitExceededException for easier catching.

Updated tests to assert at the right point and added success path coverage.

@dimas-b dimas-b requested a review from snazy March 17, 2026 14:47
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.

Add safeguard to limit commit history traversal in CommitsImpl when using offset

2 participants