Repository navigation
fix: use provider that is called at query time instead of constructor - #20537
capistrant wants to merge 1 commit into
Conversation
github-actions
Bot
added
Area - Batch Ingestion
Area - MSQ
kgyrtkirk
left a comment
kgyrtkirk
left a comment
There was a problem hiding this comment.
would be nice to have a test - but I understand that it might be more complicated than the fixing it
| protected final MemoryIntrospector memoryIntrospector; | ||
| protected final List<InputSpecSlicerProvider> inputSpecSlicerProviders; | ||
| protected final ServiceEmitter emitter; | ||
| protected final DruidNodeDiscovery dartWorkerDiscovery; |
There was a problem hiding this comment.
note: I think it might worth a try to add an annotated DruidNodeDiscovery similar to @Dart ; and declare a method do produce that in a guice module... and depend on that
that way guice will see that factorization happening and could plan it into the construction timeline of this object (or error out with some reasoning )
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found. Deferring the discovery lookup to query-context creation avoids requiring a started discovery provider during Guice construction. The provider caches the service-and-role discovery handle safely across queries, and Dart message relays request the same handle during lifecycle startup.
Reviewed 1 of 1 changed files, including adjacent discovery-provider caching, lifecycle wiring, query call sites, and controller tests. Static review; no tests were run.
This is an automated review by Codex GPT-5.6-Luna(max)
3 participants
fixes a latent bug in #20508 where eagerly setting up the discovery provider at construction time could result in a guice error. Using the provider at query time should be ok I think