Re: [PR] Switch to counter for tracking used task slots on ForkingTaskRunner (druid)

["kfaraz (via GitHub)" <gi...@apache.org>]

kfaraz commented on code in PR #18045:
URL: https://github.com/apache/druid/pull/18045#discussion_r2117421233


##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -730,12 +730,12 @@ public Map<String, Long> getIdleTaskSlotCount()
   @Override
   public Map<String, Long> getUsedTaskSlotCount()
   {
-    return ImmutableMap.of(workerConfig.getCategory(), Long.valueOf(portFinder.findUsedPortCount()));
+    return ImmutableMap.of(workerConfig.getCategory(), getUsedTaskSlotCountLong());

Review Comment:
   for brevity:
   ```suggestion
       return Map.of(workerConfig.getCategory(), getUsedTaskSlotCountLong());
   ```



##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -730,12 +730,12 @@ public Map<String, Long> getIdleTaskSlotCount()
   @Override
   public Map<String, Long> getUsedTaskSlotCount()
   {
-    return ImmutableMap.of(workerConfig.getCategory(), Long.valueOf(portFinder.findUsedPortCount()));
+    return ImmutableMap.of(workerConfig.getCategory(), getUsedTaskSlotCountLong());
   }
 
   public long getUsedTaskSlotCountLong()

Review Comment:
   It doesn't seem like this method serves any real purpose. It doesn't handle NPEs either, so we might as well just use the original method `getWorkerUsedTaskSlotCount()`. There seem to be a couple of other similar methods.



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskStorageDirTracker.java:
##########
@@ -145,6 +149,7 @@ public synchronized StorageSlot pickStorageSlot(String taskId)
       final StorageSlot candidateSlot = slots[currIncrement % slots.length];
       if (candidateSlot.runningTaskId == null) {
         candidateSlot.runningTaskId = taskId;
+        ++numUsedSlots;

Review Comment:
   For symmetry with the decrement (even though the operation already happens inside a lock), this should happen before `candidateSlot.runningTaskId` is assigned. Same comment for the other place in the code assigning this variable.



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskStorageDirTracker.java:
##########
@@ -259,4 +267,9 @@ public String toString()
              '}';
     }
   }
+
+  public synchronized long getNumUsedSlots()

Review Comment:
   Please add a short javadoc.



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskStorageDirTracker.java:
##########
@@ -259,4 +267,9 @@ public String toString()
              '}';
     }
   }
+
+  public synchronized long getNumUsedSlots()

Review Comment:
   Please add a small unit test in `TaskStorageDirTrackerTest`.



-- 
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