imply-cheddar commented on code in PR #13476:
URL: https://github.com/apache/druid/pull/13476#discussion_r1084860908
##########
extensions-contrib/kubernetes-overlord-extensions/src/test/java/org/apache/druid/k8s/overlord/KubernetesTaskRunnerTest.java:
##########
@@ -92,7 +104,10 @@
private TaskLogPusher taskLogPusher;
private DruidNode node;
- public KubernetesTaskRunnerTest()
+ private final boolean useMultipleBaseTaskDirPaths;
+
+
+ public KubernetesTaskRunnerTest(boolean useMultipleBaseTaskDirPaths)
Review Comment:
Why would the kubernetes task runner be using multiple base dirs? Isn't the
whole idea that there's a pod allocated for the task and then the pod goes
away, so there's no question of using multiple disks unless all of the
directories are passed to the task and the task actively uses multiple
directories, no?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -300,4 +338,50 @@ private String defaultDir(@Nullable String
configParameter, final String default
return configParameter;
}
+
+ private static class TaskStorageDirTracker
Review Comment:
This class doesn't belong in a Config class. A Config class should be a
config class and should not have business logic inside of it. We can see the
reasons for this in that this Config class is already doing some business logic
in figuring out the various directories with the methods
```
public File getTaskDir(String taskId)
{
return new File(baseTaskDir, IdUtils.validateId("task ID", taskId));
}
public File getTaskWorkDir(String taskId)
{
return new File(getTaskDir(taskId), "work");
}
public File getTaskTempDir(String taskId)
{
return new File(getTaskDir(taskId), "temp");
}
public File getTaskLockFile(String taskId)
{
return new File(getTaskDir(taskId), "lock");
}
```
If this Config class didn't have that business logic built into it, it would
be a lot simpler to actually extract the `StorageDirTracker`. By adding more
of this complexity here, we are going to be continuing that sin, where maybe we
should pay it back down instead?
--
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]
