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: [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]
