fix: Don't reopen closed clients in DartWorkerClientImpl. - #19927
Open
gianm wants to merge 1 commit into
Open
Conversation
DartWorkerClientImpl is scoped to a single query, so there is no need to be able to reopen clients (once a worker fails, the query also fails). This patch fixes a retry loop that could be caused when a server goes away: ControllerMessageListener#serverRemoved calls closeClient, but then the next call to the worker would cause the client to be re-created and keep retrying until its retries are exhausted.
github-actions
Bot
added
Area - Batch Ingestion
Area - MSQ
FrankChen021
reviewed
Aug 8, 2026
FrankChen021
left a comment
FrankChen021
left a comment
Member
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 5 of 5 changed files. The fix prevents closed worker clients from being reopened, but it introduces unbounded cache growth for unrelated server removals.
This is an automated review by Codex GPT-5.6-Luna(max)
| throw DruidException.defensive("%s is closed", getClass().getName()); | ||
| } | ||
|
|
||
| return clientMap.computeIfAbsent(workerId.getHostAndPort(), ignored -> makeNewClient(workerId)); |
Member
There was a problem hiding this comment.
[P2] Retains clients for unrelated node removals
DartMessageRelays invokes serverRemoved for every historical node, before controller.hasWorker(...) is checked. This computeIfAbsent therefore creates and retains a closed client/locator for nodes never used by the query; repeated node churn can grow each active query's cache until completion. Avoid retaining entries for unrelated workers while preserving the pre-first-use removal race.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DartWorkerClientImplis scoped to a single query, so there is no need to be able to reopen clients (once a worker fails, the query also fails). Prior to this patch, a client would be reopened if requested after being closed.This patch fixes a retry loop that could be caused when a server goes away:
ControllerMessageListener#serverRemovedcallscloseClient, but then the call tostopWorkerwould cause the client to be re-created and keep retrying until its retries are exhausted.