Add optional traversal limit for commit offset lookup by NssGourav · Pull Request #4007 · apache/polaris

NssGourav · 2026-03-17T08:23:13Z

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

Subham-KRLX · 2026-03-17T09:08:49Z

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.

NssGourav · 2026-03-17T09:17:32Z

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.

Subham-KRLX · 2026-04-16T18:17:33Z

@dimas-b can u please review this pr.

dimas-b · 2026-04-16T23:50:26Z

The idea looks reasonable to me at first glance... Should the PR be non-draft for a wide review, though 😉

Please also rebase (just to have a recent base commit) and fix CI :)

dimas-b · 2026-04-16T23:54:20Z

        if (tailId == offset) {
          break outer;
        }
+        if (maxTraversal != UNLIMITED_TRAVERSAL && ++traversed > maxTraversal) {


nit: please make the ++ operation more prominent (for ease of following the logic), e.g. do it on a separate line.

dimas-b · 2026-04-16T23:55:03Z

+          throw new CommitOffsetTraversalLimitExceededException(
+              refName, offset, traversed, maxTraversal);
+        }
        visited.add(tailId);


isn't traversed equivalent to visited.size()?

dimas-b · 2026-04-16T23:56:32Z

+   * <p>A value of {@code 0} disables the safeguard (unlimited traversal).
+   */
+  @WithDefault("0")
+  long commitOffsetLookupMaxTraversal();


maybe OptionalLong?

dimas-b · 2026-04-17T00:04:35Z

    var type = head.type().id();
-
-    // TODO add safeguard to limit the work done when finding the commit with ID 'offset'
+    var maxTraversal = persistence.params().commitOffsetLookupMaxTraversal();


The guardrail in this case is different in nature from commitLogReversed. Here we do not accumulate memory, AFAIK. It might be preferable to use different config settings for these cases.

Also, commitLogReversed is not called in "prod" code ATM.

dimas-b · 2026-04-17T00:12:05Z


      // Only walk, if the most recent commit ID is != offset
      if (head.id() == off) {
        return singletonList(head).iterator();


@snazy : is this correct? 🤔

Subham-KRLX · 2026-04-17T09:26:54Z

@NssGourav Could you please take a look at the recent review comments from @dimas-b and apply them to the current code? Also it looks like there are a couple of CI checks failing right now that will need to be fixed. Let us know if you need any help!

dimas-b · 2026-04-21T04:09:19Z

@NssGourav : Please rebase this PR to get recent CI job changes.

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

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

snazy · 2026-04-21T12:52:30Z

I'm still a bit concerned about adding a global setting that trickles down to all call sites (currently only maintenance).
Maintenance has its own limit and if configured with a higher value than a global one would be confusing, technically even wrong.

NssGourav · 2026-04-22T07:25:04Z

Thanks, that makes sense, a global traversal limit in persistence can definitely become confusing if maintenance already has a separate limit, and I agree that would be the wrong layering.

I’ll revise the PR to avoid introducing a global setting here and keep the safeguard scoped to the specific caller/use case instead, then I’ll rerun CI and update the PR.

github-project-automation Bot added this to Basic Kanban Board Mar 17, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Mar 17, 2026

NssGourav closed this Mar 17, 2026

NssGourav deleted the fix/commit-offset-traversal-limit branch March 17, 2026 08:44

github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Mar 17, 2026

NssGourav restored the fix/commit-offset-traversal-limit branch March 17, 2026 08:46

NssGourav reopened this Mar 17, 2026

github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Mar 17, 2026

NssGourav marked this pull request as draft March 17, 2026 08:47

NssGourav marked this pull request as ready for review March 17, 2026 08:58

dimas-b requested a review from snazy March 17, 2026 14:47

NssGourav marked this pull request as draft March 21, 2026 05:05

dimas-b reviewed Apr 17, 2026

View reviewed changes

NssGourav force-pushed the fix/commit-offset-traversal-limit branch 3 times, most recently from 6dc6db0 to 3166814 Compare April 17, 2026 14:32

NssGourav closed this Apr 17, 2026

NssGourav force-pushed the fix/commit-offset-traversal-limit branch from 3166814 to fdb5ce2 Compare April 17, 2026 14:32

github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Apr 17, 2026

NssGourav reopened this Apr 17, 2026

github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Apr 17, 2026

NssGourav added 2 commits April 21, 2026 17:16

Add optional traversal limit for commit offset lookup

5484d4f

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

Refine commit offset traversal safeguard

b626841

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 force-pushed the fix/commit-offset-traversal-limit branch from 6dc6db0 to b626841 Compare April 21, 2026 11:47

NssGourav marked this pull request as ready for review April 21, 2026 11:48

Conversation

NssGourav commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Rationale

Test plan

Uh oh!

Subham-KRLX commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NssGourav commented Mar 17, 2026

Uh oh!

Subham-KRLX commented Apr 16, 2026

Uh oh!

dimas-b commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Subham-KRLX commented Apr 17, 2026

Uh oh!

dimas-b commented Apr 21, 2026

Uh oh!

snazy commented Apr 21, 2026

Uh oh!

NssGourav commented Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NssGourav commented Mar 17, 2026 •

edited

Loading

Subham-KRLX commented Mar 17, 2026 •

edited

Loading

dimas-b commented Apr 16, 2026 •

edited

Loading