Fix capacity response in mm-less ingestion #14888

georgew5656 · 2023-08-21T18:27:26Z

Fixing bug where capacity for mm-less ingestion was not being properly reported

Description

This change: georgew5656@3954685 to report additional metrics for mm-less ingestion changed the key of the capacity hint returned in getTotalTaskSlotCount and didn't change it in OverlordResource, causing capacity to be returned as 0.

Initial plan was:
I decided to just update the hardcoded value in OverlordResource because we can't ref classes from the optional extension in core and I couldn't really find a good constants file to put the constant in for OverlordResource. Open to changing this if someone has a strong opinion here.

After some discussion I decided to move the logic for calculating capacity to the existing totalWorkerCapacity API instead of the /workers api. This cleans up the logic in the workers api (no need to pretend the k8s task runner has a worker), and lets us delete the isK8sTaskRunner method on the taskRunner.

Release note

Fix capacity response in mm-less ingestion.
Adds a new usedClusterCapacity to the GET /totalWorkerCapacity response. This API should be used to get the total ingestion capacity on the overlord.

Key changed/added classes in this PR

OverlordResource
TaskRunner,RemoteTaskRunner,HttpRemoteTaskRunner,KubernetesTaskRunner

K8s Task Runner with capacity of 100

Http Remote Task runner with a worker with capacity of 5

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
[] added integration tests.
been tested in a test Druid cluster.

YongGang

LGTM

kfaraz · 2023-08-22T04:19:24Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java

@@ -947,7 +947,7 @@ public Response apply(TaskRunner taskRunner)
                          "http",
                          "host",
                          "8100",
-                          taskRunner.getTotalTaskSlotCount().getOrDefault("taskQueue", 0L).intValue(),
+                          taskRunner.getTotalTaskSlotCount().getOrDefault("_k8s_worker_category", 0L).intValue(),


Could you please update the comment above to indicate why this specific value is required? Also, please check if the value can be brought out into a constant somewhere.

kfaraz · 2023-08-22T04:20:35Z

@georgew5656 , @YongGang , I am a little confused on why we should be using this API rather than the getTotalWorkerCapacity or getTotalTaskSlots API for this purpose.

YongGang · 2023-08-22T04:52:34Z

@kfaraz I believe this is only used by Druid console. This bug caused the following issue when run MSQ.

kfaraz

Approving for now. The correct fix would be to update the API used by web-console so that we don't need special handling for K8STaskRunner.

kfaraz

Thanks for the fix, @georgew5656 , the TaskRunner interface makes much more sense now. I have left some minor suggestions.

Could you please share a web-console screenshot after your changes to demonstrate the capacity is being populated correctly?

kfaraz · 2023-08-24T04:27:17Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java

-      workers = ImmutableList.of();
-      currentCapacity = -1;
-    }
+    Collection<ImmutableWorkerInfo> workers = taskRunner instanceof WorkerTaskRunner ?


Do we still need the list of workers?

for capacity with autoscaling, it looks like if capacity hint is not passed it tries to assume each worker has the same capacity when calculating max capacity, so it needs at least a single worker as an example.

kfaraz · 2023-08-24T04:43:49Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java

@@ -154,5 +146,14 @@ default void updateLocation(Task task, TaskLocation location)
    // do nothing
  }

+  default int getTotalCapacity()


Best add javadocs to these methods esp to point out that they can return -1 when not implemented or can't be determined.

kfaraz · 2023-08-24T04:52:49Z

@georgew5656 , also, please update the release note to reflect that a new field has been added in the API response and that this API should now be used to get total and used cluster capacity instead of the getWorkers API.

Changes: - Fix capacity response in mm-less ingestion. - Add field usedClusterCapacity to the GET /totalWorkerCapacity response. This API should be used to get the total ingestion capacity on the overlord. - Remove method `isK8sTaskRunner` from interface `TaskRunner`

…n` by changing string to key:value pair (#15207) * Fix capacity response in mm-less ingestion (#14888) Changes: - Fix capacity response in mm-less ingestion. - Add field usedClusterCapacity to the GET /totalWorkerCapacity response. This API should be used to get the total ingestion capacity on the overlord. - Remove method `isK8sTaskRunner` from interface `TaskRunner` * Using Map to perform comparison * Minor Change --------- Co-authored-by: George Shiqi Wu <george.wu@imply.io>

…n` by changing string to key:value pair (apache#15207) * Fix capacity response in mm-less ingestion (apache#14888) Changes: - Fix capacity response in mm-less ingestion. - Add field usedClusterCapacity to the GET /totalWorkerCapacity response. This API should be used to get the total ingestion capacity on the overlord. - Remove method `isK8sTaskRunner` from interface `TaskRunner` * Using Map to perform comparison * Minor Change --------- Co-authored-by: George Shiqi Wu <george.wu@imply.io>

Fix capacity response in mm-less ingestion

8db6897

YongGang approved these changes Aug 21, 2023

View reviewed changes

kfaraz reviewed Aug 22, 2023

View reviewed changes

kfaraz approved these changes Aug 22, 2023

View reviewed changes

georgew5656 added 2 commits August 23, 2023 11:52

Use capacity endpoint

64bee07

fix lint

9941a86

georgew5656 requested review from YongGang and kfaraz August 23, 2023 16:39

kfaraz approved these changes Aug 24, 2023

View reviewed changes

kfaraz added Area - Ingestion Release Notes labels Aug 24, 2023

Add javadocs

d145c0d

kfaraz merged commit ad32f84 into apache:master Aug 25, 2023
78 checks passed

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix capacity response in mm-less ingestion #14888

Fix capacity response in mm-less ingestion #14888

georgew5656 commented Aug 21, 2023 •

edited

Loading

YongGang left a comment

kfaraz Aug 22, 2023

kfaraz commented Aug 22, 2023 •

edited

Loading

YongGang commented Aug 22, 2023 •

edited

Loading

kfaraz left a comment

kfaraz left a comment

kfaraz Aug 24, 2023

georgew5656 Aug 24, 2023

kfaraz Aug 24, 2023

kfaraz commented Aug 24, 2023

Fix capacity response in mm-less ingestion #14888

Fix capacity response in mm-less ingestion #14888

Conversation

georgew5656 commented Aug 21, 2023 • edited Loading

Description

Release note

Key changed/added classes in this PR

YongGang left a comment

Choose a reason for hiding this comment

kfaraz Aug 22, 2023

Choose a reason for hiding this comment

kfaraz commented Aug 22, 2023 • edited Loading

YongGang commented Aug 22, 2023 • edited Loading

kfaraz left a comment

Choose a reason for hiding this comment

kfaraz left a comment

Choose a reason for hiding this comment

kfaraz Aug 24, 2023

Choose a reason for hiding this comment

georgew5656 Aug 24, 2023

Choose a reason for hiding this comment

kfaraz Aug 24, 2023

Choose a reason for hiding this comment

kfaraz commented Aug 24, 2023

georgew5656 commented Aug 21, 2023 •

edited

Loading

kfaraz commented Aug 22, 2023 •

edited

Loading

YongGang commented Aug 22, 2023 •

edited

Loading