Skip to content

feat: Make Coordinator client maxAttempts configurable - #20442

Open
kgyrtkirk wants to merge 7 commits into
apache:masterfrom
kgyrtkirk:coordinator-client-config
Open

kgyrtkirk wants to merge 7 commits into
apache:masterfrom
kgyrtkirk:coordinator-client-config

Conversation

@kgyrtkirk

Copy link
Copy Markdown
Member

cleanup PR to introduce a config object to enable future configuration CoordinatorClient;
maxAttempts is moved there.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fix the retry-policy regressions before merging. The new config is wired through Guice, but invalid values can create an unbounded retry policy, and the broker dynamic-config startup clients lose their former 15-attempt behavior when they switch to the shared client.

Reviewed 7 of 7 changed files.

Validation: git diff --check d2bff3c9ec541e0ce5b6e4fcdb9b79a06811cc3b f0af85f6fa1b38209dccb90933b2001b8c193f88 -- docs/configuration/index.md server/src/main/java/org/apache/druid/client/BrokerViewOfBrokerConfig.java server/src/main/java/org/apache/druid/client/BrokerViewOfCoordinatorConfig.java server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClientConfig.java server/src/main/java/org/apache/druid/rpc/guice/ServiceClientModule.java server/src/test/java/org/apache/druid/rpc/guice/ServiceClientModuleTest.java services/src/test/java/org/apache/druid/testing/embedded/EmbeddedServiceClient.java passed; no tests or build run (static review).

Severity Findings
P0 0
P1 0
P2 2
P3 0
Total 2

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

* Attempts per request to the Coordinator, the first one included, before giving up on a retryable error.
*/
@JsonProperty
private int maxAttempts = 6;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reject non-positive retry counts

Finding: The new runtime property is deserialized without a lower-bound constraint. A configured value of 0 reaches StandardRetryPolicy and makes service-client provisioning fail, while any negative value is accepted as an unlimited policy by ServiceClientImpl.shouldTry(), so a typo such as -1 turns every retryable Coordinator call into an unbounded retry and can keep operations hanging indefinitely.

Suggestion: Validate maxAttempts as a positive value at configuration binding time, and document any explicitly supported unlimited mode separately.

kgyrtkirk and others added 4 commits October 2, 2026 10:20
Add class javadoc to CoordinatorClientConfig; drop redundant prefix test and rename retry-policy test in ServiceClientModuleTest. Fewer tests, same faith.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No actionable issues found in the current head. Both prior P2 findings are resolved: @Min(1) is enforced by JsonConfigurator during Guice configuration binding, preventing non-positive retry budgets, and the default is now 15, preserving the retry budget of both broker dynamic-config startup fetches. The configured value reaches the shared Coordinator client's retry policy; the other service clients retain their existing budgets.

Reviewed 8 of 8 changed files, starting with the incremental diff and then covering the complete current PR diff, relevant configuration binding, retry scheduling, broker startup consumers, and embedded client wiring. The incremental diff includes upstream changes merged from master; those were separated from the PR changes, including the four overlapping files.

Validation: git diff --check 3e9365196f9893ff9b1cefcfbcb57d49e4080b4a HEAD passed. Static review only; tests and builds were not run.


This is an automated review by Codex GPT-5.6-Luna(max)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants