[GitHub] [druid] clintropolis commented on a change in pull request #10091: Make 0.19 brokers compatible with 0.18 router

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org