[improve][admin] Add client side looping to analyze-backlog in Topics to avoid potential HTTP call timeout#25127
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds client-side looping to the analyzeSubscriptionBacklog method to avoid HTTP call timeouts when analyzing large backlogs. Instead of relying solely on broker-side configuration limits, the client can now iteratively call the analyze-backlog endpoint and merge results until completion or a specified entry limit is reached.
- Adds new overloaded methods with a
backlogScanMaxEntriesparameter to control client-side loop termination - Implements iterative scanning logic that merges results from multiple server calls
- Includes comprehensive integration tests covering various scenarios
- Modifies OpScan behavior by removing individual deleted entry filtering
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java | Adds new API methods with backlogScanMaxEntries parameter and detailed JavaDoc documentation |
| pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java | Implements client-side looping logic with result merging and next position calculation |
| pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AnalyzeBacklogSubscriptionTest.java | Adds comprehensive integration tests for the new looping behavior |
| managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java | Removes filterReadEntries call and adjusts entry counting logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
7848fcf to
bd68a5b
Compare
lhotari
left a comment
There was a problem hiding this comment.
A few comments. I'm sorry for the delay in the review.
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
Outdated
Show resolved
Hide resolved
215bf27 to
cba3498
Compare
cba3498 to
da82b1b
Compare
lhotari
left a comment
There was a problem hiding this comment.
A few comments about the existing log messages. I think it makes sense to change them in this PR since after this PR, it would be an ordinary use case to scan larger backlogs.
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java
Outdated
Show resolved
Hide resolved
|
Thanks for your great suggestions! @lhotari |
Fixes #25083
Motivation
Use client-side looping instead of increasing broker settings to avoid potential HTTP call timeout in analyze-backlog method of Topics.
Modifications
Add client-side looping, add test.
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: oneby-wang#22