FrankChen021 commented on code in PR #20442:
URL: https://github.com/apache/druid/pull/20442#discussion_r4133448256
##########
server/src/main/java/org/apache/druid/client/BrokerViewOfBrokerConfig.java:
##########
@@ -64,27 +54,6 @@ public class BrokerViewOfBrokerConfig extends
BaseBrokerViewOfConfig<BrokerDynam
private volatile QueryConfigSnapshot querySnapshot;
@Inject
- public BrokerViewOfBrokerConfig(
- @Json final ObjectMapper jsonMapper,
- @EscalatedGlobal final ServiceClientFactory clientFactory,
- @Coordinator final ServiceLocator serviceLocator,
- final DefaultQueryConfig defaultQueryConfig
- )
- {
- this.defaultQueryConfig = defaultQueryConfig;
- this.querySnapshot = new
QueryConfigSnapshot(QueryContext.of(defaultQueryConfig.getContext()).asMap(),
null);
- this.coordinatorClient =
- new CoordinatorClientImpl(
- clientFactory.makeClient(
- NodeRole.COORDINATOR.getJsonName(),
- serviceLocator,
- StandardRetryPolicy.builder().maxAttempts(15).build()
- ),
- jsonMapper
- );
- }
-
- @VisibleForTesting
public BrokerViewOfBrokerConfig(
final CoordinatorClient coordinatorClient,
Review Comment:
## Follow-up assessment
Setting `druid.client.coordinator.maxAttempts=15` does restore the previous
attempt budget. The concern is the behavior of existing brokers that have no
value configured: the retries are spread over time, so failure on the sixth
attempt does not imply failure on the fifteenth.
`StandardRetryPolicy` starts with a 100 ms delay and caps it at 30 seconds;
`ServiceClientImpl.computeBackoffMs` doubles the delay on each retry. With
immediately failing retryable requests (for example, connection refused), six
attempts exhaust after approximately 3.1 seconds of backoff, versus 201.1
seconds for fifteen attempts, excluding request time. If the Coordinator
recovers after 5 seconds, the old policy can succeed on attempt seven at about
6.3 seconds; the new default has already failed. Both config views wait on the
request in `BaseBrokerViewOfConfig.start()`, which throws on failure and aborts
broker startup.
I therefore still consider the default change a P2 startup-availability
regression and recommend preserving the previous retry budget for these two
startup fetches while retaining configurability. This is one reconfirmed
finding, with no new findings; this follow-up does not reassess the separate
configuration-value finding or the whole PR.
Reviewed 7 files for this follow-up: 4 of the 7 changed files plus 3
supporting files:
-
`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/main/java/org/apache/druid/client/BaseBrokerViewOfConfig.java`
- `server/src/main/java/org/apache/druid/rpc/StandardRetryPolicy.java`
- `server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java`
Validation: static tracing of retry scheduling and startup failure
propagation, plus a small arithmetic check of the backoff sums. No build or
tests run.
---
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the finding or replying to the comment, you can request
another review from me to trigger a new automated review.
<!-- mergelens:review -->
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
