clintropolis commented on a change in pull request #10091:
URL: https://github.com/apache/druid/pull/10091#discussion_r447326189
##########
File path: server/src/main/java/org/apache/druid/guice/StorageNodeModule.java
##########
@@ -74,17 +82,36 @@ public DruidServerMetadata getMetadata(
@Provides
@LazySingleton
- public DataNodeService getDataNodeService(@Nullable ServerTypeConfig
serverTypeConfig, DruidServerConfig config)
+ public DataNodeService getDataNodeService(
+ @Nullable ServerTypeConfig serverTypeConfig,
+ DruidServerConfig config,
+ @Named(IS_SEGMENT_CACHE_CONFIGURED) Boolean isSegmentCacheConfigured
+ )
{
if (serverTypeConfig == null) {
- throw new ProvisionException("Must override the binding for
ServerTypeConfig if you want a DruidServerMetadata.");
+ throw new ProvisionException("Must override the binding for
ServerTypeConfig if you want a DataNodeService.");
+ }
+ if (!isSegmentCacheConfigured) {
+ log.info("Segment cache not configured on ServerType [%s]",
serverTypeConfig.getServerType());
Review comment:
nit: this log should probably include the implications, such as it will
not be assignable for segment placement.
##########
File path: services/src/main/java/org/apache/druid/cli/ServerRunnable.java
##########
@@ -194,15 +202,40 @@ private DiscoverySideEffectsProvider(
this.useLegacyAnnouncer = useLegacyAnnouncer;
}
+ @VisibleForTesting
+ DiscoverySideEffectsProvider(
Review comment:
Since this is a release blocker for 0.19, I don't think refactoring is
necessary. FWIW I don't really like AssistedInject, it's a bit too magical for
my taste.
##########
File path: services/src/main/java/org/apache/druid/cli/ServerRunnable.java
##########
@@ -194,15 +202,40 @@ private DiscoverySideEffectsProvider(
this.useLegacyAnnouncer = useLegacyAnnouncer;
}
+ @VisibleForTesting
+ DiscoverySideEffectsProvider(
+ final NodeRole nodeRole,
+ final List<Class<? extends DruidService>> serviceClasses,
+ final boolean useLegacyAnnouncer,
+ final DruidNode druidNode,
+ final DruidNodeAnnouncer announcer,
+ final ServiceAnnouncer legacyAnnouncer,
+ final Lifecycle lifecycle,
+ final Injector injector
+ )
+ {
+ this.nodeRole = nodeRole;
+ this.serviceClasses = serviceClasses;
+ this.useLegacyAnnouncer = useLegacyAnnouncer;
+ this.druidNode = druidNode;
+ this.announcer = announcer;
+ this.legacyAnnouncer = legacyAnnouncer;
+ this.lifecycle = lifecycle;
+ this.injector = injector;
+ }
+
@Override
public Child get()
{
ImmutableMap.Builder<String, DruidService> builder = new
ImmutableMap.Builder<>();
for (Class<? extends DruidService> clazz : serviceClasses) {
DruidService service = injector.getInstance(clazz);
- builder.put(service.getName(), service);
+ if (service.isDiscoverable()) {
+ builder.put(service.getName(), service);
+ } else {
+ log.info("Service[%s] is not discoverable", service.getName());
Review comment:
nit: this log should also maybe be more informative that the service is
being skipped and will not be listed as a service the node provides i think
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
