Add maximumCapacity to taskRunner #17107
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kfaraz
reviewed
Sep 23, 2024
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
Outdated
Show resolved
Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
Outdated
Show resolved
Hide resolved
| @@ -1648,6 +1649,30 @@ public int getTotalCapacity() | |||
| return getWorkers().stream().mapToInt(workerInfo -> workerInfo.getWorker().getCapacity()).sum(); | |||
| } | |||
|
|
|||
| @Override | |||
| public int getMaximumCapacity() | |||
There was a problem hiding this comment.
Maybe add a javadoc here listing the cases when this method returns -1.
There was a problem hiding this comment.
I wonder if this method body should be commoned out between RemoteTaskRunner and HttpTaskRunner.
There was a problem hiding this comment.
were you thinking of introducing a new class to do this?
...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java
Outdated
Show resolved
Hide resolved
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueryTool.java
Outdated
Show resolved
Hide resolved
suneet-s
reviewed
Sep 23, 2024
...rd-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesAndWorkerTaskRunner.java
Outdated
Show resolved
Hide resolved
kfaraz
approved these changes
Oct 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move logic for calculating maximumCapacity (tatal capacity based on max workers from autoscaling) to task runners
Description
It is possible to get weird responses from the /totalWorkerCapacity endpoint if mmless ingestion is enabled and the overlord dynamic.autoscaler config is set. This is because the TaskQueryTool.getTotalWorkerCapacity gets totalCapacity from the overlord's task runner but gets maximumCapacity directly from the dynamic config.
I think it makes sense to just expose a getMaximumCapacity method on TaskRunner that defaults to -1 (the default value of maximumCapacity) and gets overwritten.
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
In order to move getMaximumCapacity logic into each TaskRunner, i had to duplicate logic between HttpRemoteTaskRunner and RemoteTaskRunner since they have the same logic (check the overlord dynamic configuration), but I think this is okay in order for the task runners to be responsible for their own max capacity values.
Release note
Key changed/added classes in this PR
TaskRunnerHttpRemoteTaskRunner/RemoteTaskRunnerKubernetesAndWorkerTaskRunner/KubernetesTaskRunnerTaskQueryToolThis PR has: