[improve][cli] Add client side looping in "pulsar-admin topics analyze-backlog" cli to avoid potential HTTP call timeout#25126
Conversation
|
@oneby-wang #25127 has been merged |
oneby-wang
force-pushed
the
pulsar_cli_client_side_analyze_backlog
branch
from
b48e0eb to
83bdbf1
Compare
oneby-wang
force-pushed
the
pulsar_cli_client_side_analyze_backlog
branch
3 times, most recently
from
425e8bf to
941b50c
Compare
oneby-wang
force-pushed
the
pulsar_cli_client_side_analyze_backlog
branch
from
717742a to
91001d3
Compare
oneby-wang
force-pushed
the
pulsar_cli_client_side_analyze_backlog
branch
from
91001d3 to
2456a05
Compare
|
Hi @lhotari, could you please help review this refactored PR? |
| cmdTopics.run(split("analyze-backlog persistent://myprop/ns1/ds1 -s sub1")); | ||
| verify(mockTopics).analyzeSubscriptionBacklog("persistent://myprop/ns1/ds1", "sub1", | ||
| Optional.empty()); | ||
|
|
There was a problem hiding this comment.
it could be useful to keep a unit test for parameter parsing since this would catch issues before the integration test is run
| @Option(names = {"--plain-print", | ||
| "-pp"}, description = "Plain(Non-pretty) print the final result output as NDJSON", required = false) |
There was a problem hiding this comment.
since there's already -p, it's not recommended to add -pp. Instead, it's better to shorten --plain-print to --plain and just not add a short option name at all.
| getTopics().analyzeSubscriptionBacklogAsync(persistentTopic, subName, startPosition, result -> { | ||
| boolean terminate = result.getEntries() >= backlogScanMaxEntries; | ||
| if (!quiet && !terminate) { | ||
| print(result, false); | ||
| } | ||
| return terminate; | ||
| }).get(); |
There was a problem hiding this comment.
using the async method isn't useful since .get() is called. There would be a need to change to throws Exception when using analyzeSubscriptionBacklog method.
| @Option(names = {"--backlog-scan-max-entries", | ||
| "-b"}, description = "The maximum number of backlog entries the client will scan before terminating " | ||
| + "its loop", required = false) | ||
| private long backlogScanMaxEntries = -1; |
There was a problem hiding this comment.
--backlog-scan-max-entries default of -1 relies on a subtle side-effect for backward compat — CmdTopics.java:3019 With -1, the predicate result.getEntries() >= -1 is always true, so the loop completes on the first iteration (matching old single-call behavior). That works, but it conflates "unset" with "terminate immediately." Preferred: treat unset as "no cap" (use the no-predicate overload analyzeSubscriptionBacklogAsync(topic, sub, pos)) and only take the looping path when -b is supplied.
That makes intent explicit and avoids relying on entries >= -1 semantics. Also add a validation error for -b 0 or negative user input, since those are meaningless.
2 participants
Fixes #25537
Motivation
Use client-side looping instead of increasing broker settings to avoid potential HTTP call timeout in "pulsar-admin topics analyze-backlog" cli.
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#21