LakshSingla commented on code in PR #14527:
URL: https://github.com/apache/druid/pull/14527#discussion_r1254323579
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/guice/MSQDurableStorageModule.java:
##########
@@ -98,6 +99,9 @@ public void configure(Binder binder)
.addBinding()
.to(DurableStorageCleaner.class);
}
+ } else {
+ // bind with nil implementation so that configs are not required during
service startups.
+ binder.bind(Key.get(StorageConnector.class,
MultiStageQuery.class)).toInstance(NilStorageConnector.getInstance());
Review Comment:
In that case
1. We should selectively bind the implementation to the Broker Nodes only.
For MSQ stuff residing in the indexers, we have an extensive system in place
that determines whether durable storage is enabled.
2. This still seems slightly out of place, where each method throws a UOE
for the user. IMO we should do something like this: Wherever we are using a
storage connector in the broker, we should check `isDurableStorageEnabled` - as
we do in the MSQ engine - which would basically check if the connector is a
`NilStorageConnector` and then throw an error which is directed at the
ADMIN/OPERATOR of the cluster.
3. The UOE in the connector's implementation should be a defensive
mechanism, and we should ensure that it is never called.
--
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]
