Add sequential sketch merging to MSQ by adarshsanjeev · Pull Request #13205 · apache/druid

adarshsanjeev · 2022-10-11T02:32:30Z

Current Implementation

In the current MSQ implementation, each worker maintains a ClusterByStatisticsCollector. If we are clustering by time, one sketch is maintained for each time chunk. During the merge, we sent the entire ClusterByStatisticsCollector from the worker to the controller. These are merged together, downsampling if memory taken is too much and partitions are generated from this.

Potential Improvements

Taking all worker sketches into controller memory is an unneeded step, which leads to downsampling of sketches and generating partitions outside the targeted weight. Since we have a primary partitioning on time, we only need to maintain the sketch from all workers for a particular time partition, generate the partition and remove the sketches. This is the sequential sketch merging approach.

Since this increases time taken however, this has been moved to a separate WorkerSketchFetcher. This will use an executor service to avoid blocking the main controller thread. This will not remove the current functionality, it will only uses sequential merging under cases that it is likely to provide greater benefits. For small sketches, it continues to fetch the entire sketch for the speed.

Key changes

Add a new phase to controller: MERGING_STATISTICS and necessary transitions
Change worker to now send a WorkerReport to the controller, which contains information necessary to fetch sketches later.
Change the sketch fetching a pull from the controller instead of a push from the worker.
Add WorkerSketchFetcher to fetch sketches in the background.

Context changes

Add a new parameter clusterStatisticsMergeMode. It controls whether to parallel or sequential merging of worker sketches. Can be PARALLEL, SEQUENTIAL or AUTO. On AUTO tries to find the best approach based on number of workers and size of input rows.

Release note

Removes an API on ControllerChatHandler /keyStatistics/{queryId}/{stageNumber}/{workerNumber}
Adds an API on ControllerChatHandler /partialKeyStatisticsInformation/{queryId}/{stageNumber}/{workerNumber} that accepts an object that is deserializable to PartialKeyStatisticsInformation.

Key changed/added classes in this PR

ClusterByStatisticsCollector
WorkerClient
ControllerClient
StageDefinition
WorkerSketchFetcher
ControllerStageTracker
ClusterByStatisticsWorkerReport

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.
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.

…erging

cryptoe

A partial review. Will try to get this finished tomorrow.

LakshSingla

Thanks for the contribution! Left a few comments on the same.

adarshsanjeev · 2022-11-14T10:31:58Z

+ */
+public class WorkerSketchFetcher
+{
+  private static final int DEFAULT_THREAD_COUNT = 10;


Going through this again, with regards to memory usage, we should limit the threads in relation to the number of sketches the controller can maintain at a time in memory.

good catch. Yeah, we need to limit the number of parallel threads here. Lets start with 4 ?

Changed to 4

cryptoe

Left some comments.

cryptoe · 2022-11-14T12:17:34Z

+ */
+public class WorkerSketchFetcher
+{
+  private static final int DEFAULT_THREAD_COUNT = 10;


good catch. Yeah, we need to limit the number of parallel threads here. Lets start with 4 ?

…erging

cryptoe

Left my final review. Thanks for waiting on this @adarshsanjeev .

cryptoe

Overall lgtm. Some comments here and there.

…erging

cryptoe · 2022-11-22T04:27:10Z

Thanks for the contribution @adarshsanjeev.

* Add sketch fetching framework * Refactor code to support sequential merge * Update worker sketch fetcher * Refactor sketch fetcher * Refactor sketch fetcher * Add context parameter and threshold to trigger sequential merge * Fix test * Add integration test for non sequential merge * Address review comments * Address review comments * Address review comments * Resolve maxRetainedBytes * Add new classes * Renamed key statistics information class * Rename fetchStatisticsSnapshotForTimeChunk function * Address review comments * Address review comments * Update documentation and add comments * Resolve build issues * Resolve build issues * Change worker APIs to async * Address review comments * Resolve build issues * Add null time check * Update integration tests * Address review comments * Add log messages and comments * Resolve build issues * Add unit tests * Add unit tests * Fix timing issue in tests

Since apache#13205, a special deserializer module has no longer been necessary to read key collector snapshots. This patch removes the unnecessary code.

Since #13205, a special deserializer module has no longer been necessary to read key collector snapshots. This patch removes the unnecessary code.

adarshsanjeev added 5 commits October 10, 2022 20:49

Add sketch fetching framework

681f8fb

Refactor code to support sequential merge

b375d59

Update worker sketch fetcher

113a3d9

Refactor sketch fetcher

9fce02a

Refactor sketch fetcher

1657ee0

adarshsanjeev marked this pull request as ready for review October 14, 2022 03:17

kfaraz added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Oct 14, 2022

adarshsanjeev added 4 commits October 17, 2022 16:37

Add context parameter and threshold to trigger sequential merge

313e9a8

Fix test

0be3d4f

Add integration test for non sequential merge

a312f20

Merge remote-tracking branch 'origin/master' into sequential-sketch-m…

8037413

…erging

cryptoe reviewed Oct 25, 2022

View reviewed changes

adarshsanjeev added 3 commits October 26, 2022 09:25

Address review comments

c74da2c

Address review comments

5ec8e39

Address review comments

0264bb0

LakshSingla reviewed Nov 7, 2022

View reviewed changes

adarshsanjeev commented Nov 14, 2022

View reviewed changes

cryptoe reviewed Nov 14, 2022

View reviewed changes

adarshsanjeev added 9 commits November 14, 2022 21:42

Merge remote-tracking branch 'origin/master' into sequential-sketch-m…

6395b7c

…erging

Resolve maxRetainedBytes

4656f47

Add new classes

07876f5

Renamed key statistics information class

bb86c19

Rename fetchStatisticsSnapshotForTimeChunk function

35d8e71

Address review comments

3a2078f

Address review comments

bcaeae9

Update documentation and add comments

18c60bc

Resolve build issues

2f91002

cryptoe reviewed Nov 16, 2022

View reviewed changes

Comment thread extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerClient.java Outdated

adarshsanjeev added 2 commits November 16, 2022 15:08

Resolve build issues

3852cd1

Change worker APIs to async

682aa71

adarshsanjeev added 3 commits November 16, 2022 19:07

Address review comments

85ad968

Resolve build issues

090ce07

Add null time check

aa58285

cryptoe reviewed Nov 17, 2022

View reviewed changes

adarshsanjeev added 3 commits November 17, 2022 10:18

Update integration tests

522c090

Address review comments

953ff05

Add log messages and comments

3efa5b6

cryptoe reviewed Nov 17, 2022

View reviewed changes

adarshsanjeev added 2 commits November 17, 2022 21:12

Resolve build issues

7514cee

Add unit tests

08d5f9c

cryptoe reviewed Nov 18, 2022

View reviewed changes

Comment thread ...ions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerSketchFetcher.java Outdated

adarshsanjeev added 3 commits November 18, 2022 19:19

Add unit tests

4c7e2db

Merge remote-tracking branch 'origin/master' into sequential-sketch-m…

894d0d4

…erging

Fix timing issue in tests

837ed3d

cryptoe approved these changes Nov 21, 2022

View reviewed changes

cryptoe merged commit 280a0f7 into apache:master Nov 22, 2022

cryptoe added this to the 25.0 milestone Nov 22, 2022

cryptoe added Backport and removed Backport labels Nov 22, 2022

adarshsanjeev mentioned this pull request Dec 1, 2022

Remove stray reference to fix OOM while merging sketches #13475

Merged

10 tasks

cryptoe mentioned this pull request Dec 13, 2022

Multiple fixes for the MSQ stats merging piece #13463

Merged

10 tasks

This was referenced Dec 18, 2022

[Draft] 25.0.0 Release Notes #13592

Closed

Add SegmentAllocationQueue to batch allocation actions #13369

Merged

gianm mentioned this pull request Mar 13, 2024

MSQ: Remove unnecessary snapshot deserialization code. #16116

Merged

vogievetsky pushed a commit that referenced this pull request Mar 18, 2024

MSQ: Remove unnecessary snapshot deserialization code. (#16116)

36bc94c

Since #13205, a special deserializer module has no longer been necessary to read key collector snapshots. This patch removes the unnecessary code.

Conversation

adarshsanjeev commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Implementation

Potential Improvements

Key changes

Context changes

Release note

Key changed/added classes in this PR

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adarshsanjeev Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

cryptoe Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

adarshsanjeev Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adarshsanjeev commented Oct 11, 2022 •

edited

Loading