[GitHub] [druid] paul-rogers commented on a diff in pull request #13930: add null safety checks for DiscoveryDruidNode services for more resilient http server and task views

["paul-rogers (via GitHub)" <gi...@apache.org>]

paul-rogers commented on code in PR #13930:
URL: https://github.com/apache/druid/pull/13930#discussion_r1139445564


##########
server/src/main/java/org/apache/druid/server/coordination/ServerType.java:
##########
@@ -149,6 +149,18 @@ public static ServerType fromString(String type)
     return ServerType.valueOf(StringUtils.toUpperCase(type).replace('-', '_'));
   }
 
+  public static ServerType fromNodeRole(NodeRole nodeRole)
+  {
+    // this doesn't actually check that the NodeRole is a typical data node
+    if (nodeRole.equals(NodeRole.HISTORICAL)) {
+      return ServerType.HISTORICAL;
+    } else if (nodeRole.equals(NodeRole.BROKER)) {
+      return ServerType.BROKER;
+    } else {
+      return ServerType.INDEXER_EXECUTOR;

Review Comment:
   Should this handle the actual indexer executor roles? If I were to call this with `NodeRole.COORDINATOR`, I'd learn that id is an indexer executor?



##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java:
##########
@@ -565,13 +566,26 @@ public void nodeViewInitialized()
 
   private Worker toWorker(DiscoveryDruidNode node)
   {
+    final WorkerNodeService workerNodeService = node.getService(WorkerNodeService.DISCOVERY_SERVICE_KEY, WorkerNodeService.class);
+    if (workerNodeService == null) {
+      // this shouldn't typically happen, but just in case it does, make a dummy worker to allow the callbacks to
+      // continue since addWorker/removeWorker only need worker.getHost()

Review Comment:
   This kind of thing is a great short-term fix, but seems like a ticking time bomb. Some later bit of code will call this expecting to get a real `Worker`. Should the code instead either a) change to just do `getNodeHost()`, or if it really is a `toWorker()`, return `null` if there isn't actually a worker?



-- 
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: commits-unsubscribe@druid.apache.org

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