You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "Subham-KRLX (via GitHub)" <gi...@apache.org> on 2026/03/15 18:20:06 UTC
[I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Subham-KRLX opened a new issue, #4000:
URL: https://github.com/apache/polaris/issues/4000
### Describe the bug
When using the NoSQL persistence implementation, CommitsImpl traverses commit history to honor an offset without any explicit safeguard on how much history it will walk. The code contains TODO comments about adding a safeguard for this, but none is implemented. This can lead to unbounded work and potential performance / resource issues for large histories or pathological offset values.
### To Reproduce
Use the NoSQL persistence implementation with a commit log that has many entries.
Call an API that ultimately uses CommitsImpl with an offset pointing far back in history.
CommitsImpl walks the commit chain from head towards the offset with no traversal limit.
The operation can become very expensive and tie up resources.
### Actual Behavior
The commit lookup by offset walks the commit history from the head with no bound on the number of commits it will traverse. There are TODO comments in CommitsImpl:
// TODO add safeguard to limit the work done when finding the commit with ID 'offset'
### Expected Behavior
Commit history traversal to satisfy an offset should be bounded and controlled:
A clear, documented maximum traversal limit (constant or configuration).
If the limit is exceeded before finding the requested offset, the operation should fail fast with a well‑defined error/status instead of continuing unbounded work.
### Additional context
File:persistence/nosql/persistence/impl/src/main/java/org/apache/polaris/persistence/nosql/impl/commits/CommitsImpl.java
There are at least two TODOs calling out the missing safeguard.
I would like to work on this issue by:
Introducing a configurable traversal limit for offset lookup.
Adding tests that cover both normal offsets and the “limit exceeded” case.
### System information
OS: macOS
Polaris Catalog Version: (current `main` / latest at time of report)
Object storage & setup: Not specific to a particular backend; this is about the NoSQL commit traversal logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "NssGourav (via GitHub)" <gi...@apache.org>.
NssGourav commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4065668029
i can work on this issue!.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4067402997
Yup! You're totally right, there are TODOs in `CommitsImpl` to eventually add safeguards.
The only call sites that currently use the `org.apache.polaris.persistence.nosql.api.commit.Commits` interface are the correctness tests and the maintenance service. The latter already has implicit safeguards via its configuration(s).
We do not have plans to expose the commit log in public facing APIs. The former is literally "just" a test that runs against a well-defined data set.
Guardrails and back-pressure mechanisms play "hand in hand" to protect the system against "over-usage."
The TODOs in `CommitsImpl` were actually meant to have such guardrails at the call sites of the functionality, potentially in combination w/ back-pressure mechanisms.
Some operations, like the maintenance service, can and should be rate-limited. Such background operations may have legit reasons to walk the whole log. Having a hard limit could break the maintenance service.
Operational requests, think: something that comes in via a REST API request, should have proper guardrails in place - both in terms of request/response payload size and operation runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "NssGourav (via GitHub)" <gi...@apache.org>.
NssGourav commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4065711493
From my understanding, both `commitLog` and `commitLogReversed` traverse commit history without any upper bound while resolving offsets. This can lead to unbounded work when the offset is far back or missing, causing potential performance issues.
I’m planning to introduce a configurable traversal limit (via `PersistenceParams`) and enforce it in both methods with a fail-fast mechanism.
Does this approach align with the expected solution? Happy to adjust based on your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "Subham-KRLX (via GitHub)" <gi...@apache.org>.
Subham-KRLX commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4063612750
I have identified the specific loops in CommitsImpl.java where the traversal safeguards are currently missing. I'm working on a fix that introduces a configurable maxCommitHistoryTraversalCount and adds unit tests for both valid and 'limit exceeded' scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "NssGourav (via GitHub)" <gi...@apache.org>.
NssGourav commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4069862211
Hello @snazy
Based on your feedback, I’ll avoid adding a hard traversal limit directly inside CommitsImpl, since that could break legitimate full log operations such as the maintenance service.
Instead, I’ll keep `CommitsImpl` capable of walking the entire history and introduce guardrails at the call sites that are exposed to user/operational requests. Concretely, that would mean:
Enforcing offset / traversal limits and payload size limits in the APIs or services that wrap Commits (e.g., REST/admin operations), possibly with configuration for those limits.
Leaving maintenance/background operations either unbounded or governed by their own, much higher limits and rate limiting / back pressure.
I’ll draft a change that:
- Adds these safeguards at the API/service layer instead of inside `CommitsImpl`.
- Ensures tests and the maintenance service continue to work unchanged (or with explicit “unbounded” `behavior` where appropriate).
- Does this direction match what you had in mind for the TODOs in `CommitsImpl`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Add safeguard to limit commit history traversal in CommitsImpl when using offset [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on issue #4000:
URL: https://github.com/apache/polaris/issues/4000#issuecomment-4256955471
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment if you think it's still relevant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org