perf: improve RemoteTaskRunner task assignment loop performance by jasonk000 · Pull Request #12096 · apache/druid

jasonk000 · 2021-12-23T21:00:09Z

Description

Improve the performance of RemoteTaskRunner::tryAssignTask which consumes long periods of CPU on the Overlord during a task restart operation.

Screenshot of profiler showing long period of rtr-pending-.. task thread.

Screenshot of profile flamegraph for this thread, showing 100pc of CPU in tryAssignTask loop:

Key changed/added classes in this PR

This change:

eliminates triple nested call of getRunningTasks() in ZkWorker::toImmutable, and,
reduces the work performed in ZkWorker::isRunningTask by parsing only the id field instead of the entire ZkWorker json.

By eliminating this extra work, the loop is much tighter.

This is a change coupled to this mailing thread discussion:
https://lists.apache.org/thread/9jgdwrodwsfcg98so6kzfhdmn95gzyrj

Tests

Tests in RemoteTaskRunner*Test.java capture this functionality.

This PR has:

been self-reviewed.
been tested in a test Druid cluster (as a part of a larger block of changes).

FrankChen021 · 2021-12-24T01:52:38Z

Good catch. Overall LGTM. Leave a comment to you to see if we can improve more.

FrankChen021 · 2021-12-24T08:38:13Z

+            return parser.getText();
+          }
+        }
+        return null;


what if a null is returned? will it result in exception in the getRunningTaskIds?

It will not result in an exception in getRunningTaskIds, but it would allow null to be in the result set. I think from this class perspective is valid (if a task exists with a null id, then getRunningTaskIds should include null). And, TaskAnnouncement currently would (should?) exhibit this behaviour on deserialization.

However, I am not clear on the right system-wide behaviour here. If all tasks should have valid Ids, then I think the clearer least-surprising behaviour is to explicitly throw an exception, or maybe filter null elements out of the return value from getRunningTaskIds.

What do you think?

I don't know if there's a case that there's no id field in the task. Maybe it only exists in our code here in theory. Since here is a performance critical path based on your investigation, I think we can leave the null here.

samarthjain · 2022-02-25T08:10:02Z

Some minor comments, otherwise looks good.

+1 after the above are addressed and CI passes.

jasonk000 · 2022-02-28T19:30:25Z

HI @samarthjain @xvrl , I believe these comments have all been addressed in a633c74. I included a set of unit tests to confirm that any particular structure should be correctly read, so, I think this is good to go?

samarthjain · 2022-02-28T19:46:22Z

HI @samarthjain @xvrl , I believe these comments have all been addressed in a633c74. I included a set of unit tests to confirm that any particular structure should be correctly read, so, I think this is good to go?

Hi, @jasonk000 - sorry, it looks like I forgot publishing my review comments. They are mostly minor.

This improves the performance of the ZkWorker task lookup loop by eliminating repeat calls to getRunningTasks() in toImmutable(), and reduces the work performed in isRunningTask() to stream-parse the id field instead of entire JSON blob.

jasonk000 · 2022-03-01T19:41:37Z

@samarthjain squashed & rebased

xvrl · 2022-03-01T22:06:04Z

+
+import java.util.function.Function;
+
+public class ZkWorkerTest


can we add a test where we serialize the task info using jackson and then deserialize with the task id extractor? this will ensure we update this code whenever the task serialization format changes.

added test in 9a2175d

Include a test case that explicitly serializes a TaskAnnouncement and uses the new standalone task extractor.

samarthjain · 2022-03-02T17:38:11Z

+1. Thank you for your work, @jasonk000 .

Taking the work done in apache#12096 a little further: 1) Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner). The original patch only helped with the ZkWorker (RemoteTaskRunner). 2) Improve the ZkWorker version somewhat by avoiding multiple iterations through the task announcements map.

…#14546) * More efficient generation of ImmutableWorkerHolder from WorkerHolder. Taking the work done in #12096 a little further: 1) Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner). The original patch only helped with the ZkWorker (RemoteTaskRunner). 2) Improve the ZkWorker version somewhat by avoiding multiple iterations through the task announcements map. * Pick better names and use better logic. * Only runnable tasks. * Fix test. * Fix testBlacklistZKWorkers50Percent.

…apache#14546) * More efficient generation of ImmutableWorkerHolder from WorkerHolder. Taking the work done in apache#12096 a little further: 1) Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner). The original patch only helped with the ZkWorker (RemoteTaskRunner). 2) Improve the ZkWorker version somewhat by avoiding multiple iterations through the task announcements map. * Pick better names and use better logic. * Only runnable tasks. * Fix test. * Fix testBlacklistZKWorkers50Percent.

jasonk000 mentioned this pull request Dec 23, 2021

perf: eliminate double string concat in remote-task-runner shutdown logging #12097

Merged

3 tasks

FrankChen021 reviewed Dec 24, 2021

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java Outdated

FrankChen021 added the Performance label Dec 24, 2021

FrankChen021 reviewed Dec 24, 2021

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java Outdated

FrankChen021 reviewed Dec 24, 2021

View reviewed changes

FrankChen021 approved these changes Dec 25, 2021

View reviewed changes

jasonk000 mentioned this pull request Dec 31, 2021

Kafka ingestion lag spikes up whenever tasks are rolling #11414

Open

xvrl reviewed Jan 6, 2022

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java

xvrl reviewed Jan 6, 2022

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java

xvrl reviewed Jan 6, 2022

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java Outdated

xvrl reviewed Jan 6, 2022

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java

samarthjain reviewed Feb 28, 2022

View reviewed changes

perf: improve ZkWorker task lookup performance

e41ded2

This improves the performance of the ZkWorker task lookup loop by eliminating repeat calls to getRunningTasks() in toImmutable(), and reduces the work performed in isRunningTask() to stream-parse the id field instead of entire JSON blob.

jasonk000 force-pushed the zkworker-parsing branch from 6603e32 to e41ded2 Compare March 1, 2022 19:40

xvrl reviewed Mar 1, 2022

View reviewed changes

perf: improve ZkWorker task lookup performance (feedback)

9a2175d

Include a test case that explicitly serializes a TaskAnnouncement and uses the new standalone task extractor.

samarthjain merged commit f594e7a into apache:master Mar 2, 2022

jasonk000 deleted the zkworker-parsing branch March 2, 2022 17:51

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

gianm mentioned this pull request Jul 7, 2023

More efficient generation of ImmutableWorkerHolder from WorkerHolder. #14546

Merged


		import java.util.function.Function;

		public class ZkWorkerTest

Conversation

jasonk000 commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Tests

Uh oh!

Uh oh!

FrankChen021 commented Dec 24, 2021

Uh oh!

Uh oh!

FrankChen021 Dec 24, 2021

Choose a reason for hiding this comment

Uh oh!

jasonk000 Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Dec 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samarthjain commented Feb 25, 2022

Uh oh!

jasonk000 commented Feb 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samarthjain commented Feb 28, 2022

Uh oh!

jasonk000 commented Mar 1, 2022

Uh oh!

xvrl Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

jasonk000 Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

samarthjain commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jasonk000 commented Dec 23, 2021 •

edited

Loading

jasonk000 Dec 24, 2021 •

edited

Loading