Use hash of Segment IDs instead of a list of explicit segments in auto compaction by jihoonson · Pull Request #8571 · apache/druid

jihoonson · 2019-09-23T07:02:29Z

Description

Currently when the coordinator issues a compaction task in auto compaction, it specifies a list of segments to compact explicitly. The list of segments is used to validate the given segments are still the most recent segments in compaction task.

This could lead to a very large compaction task spec which could be larger than the max znode size of ZooKeeper. To avoid this problem, auto compaction supports a configuration of maxNumSegmentsToCompact which limits the number of segments to compact together at the same time. However, with this way, the auto compaction has a limitation that it cannot compact an interval if there are too many segments.

This PR is to avoid this issue by using a hash of segment IDs instead of the list of segments for validating input segments. The below changes are also included.

New IOConfig for compaction task

Compaction task now requires an ioConfig. You can set inputSpec in the ioConfig. An example ioConfig is:

  "ioConfig" : {
    "type": "compact",
    "inputSpec": {
      "type": "interval",
      "interval": "2017-01-01/2018-01-01"
    }
  }

There are two types of inputSpecs, i.e., interval and segments, for now.

    "inputSpec": {
      "type": "interval",
      "interval": "2017-01-01/2018-01-01"
    }

    "inputSpec": {
      "type": "segments",
      "segments": ["segmentId1", "segmentId2", ...]
    }

Using interval inputSpec for auto compaction

Auto compaction used to specify all segments to compact explicitly in the compaction task spec. Now it always uses the interval inputSpec instead. maxNumSegmentsToCompact was dropped as well.

This PR has:

been self-reviewed.
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 unit tests or modified existing tests to cover new code paths.
been tested in a test Druid cluster.

ccaominh · 2019-10-02T00:29:56Z

 @PublicApi
 public class SegmentUtils
 {
+  private static final HashFunction HASH_FUNCTION = Hashing.sha256();


If the hash does not need to be cryptographically secure, perhaps Hashing.murmur3_128() is a better option.

(If you change the hash function, the comment on 49 and CompactionIntervalSpec need to be updated too.)

Hmm, why is it better?

Cryptographically-secure hash functions are typically slower since they need to more work to achieve that property. For example, this perf test observed sha256 to be about an order of magnitude slower than murmur3: https://rusty.ozlabs.org/?p=511

ccaominh · 2019-10-02T00:32:01Z


+### Compaction IOConfig
+
+The compaction IOConfig requires to specify `inputSpec` as seen below.


Suggestion: to specify -> specifying

Fixed, thanks.

ccaominh · 2019-10-02T00:37:51Z

+    if (ioConfig != null) {
+      this.ioConfig = ioConfig;
+    } else {
+      if (interval != null) {
+        this.ioConfig = new CompactionIOConfig(new CompactionIntervalSpec(interval, null));
+      } else if (segments != null && !segments.isEmpty()) {
+        this.ioConfig = new CompactionIOConfig(SpecificSegmentsSpec.fromSegments(segments));
+      } else {
+        throw new IAE("Missing ioConfig");
+      }
    }


This behavior is bit different from before (e.g., it allows both interval and segments to be not null). Perhaps it's better to enforce that exactly one of interval, segments, or ioConfig is not null, which has similar behavior to the old code.

ccaominh · 2019-10-02T00:40:19Z

+import java.util.Objects;
+
+@JsonTypeName("compact")
+public class CompactionIOConfig implements IOConfig


Do you want to add a javadoc for the class?

ccaominh · 2019-10-02T00:50:16Z

+import java.util.Objects;
+import java.util.stream.Collectors;
+
+public class SpecificSegmentsSpec implements CompactionInputSpec


Do you want to add a javadoc for the class?

I think this class is pretty obvious.

ccaominh · 2019-10-02T01:05:10Z

+  }
+
+  @JsonCreator
+  public SpecificSegmentsSpec(@JsonProperty("segments") List<String> segments)


Is there a serde test for this?

Yes, it's in CompactionTaskTest.

ccaominh · 2019-10-02T01:09:10Z

+    Assert.assertEquals(expected.getIoConfig(), actual.getIoConfig());
    Assert.assertEquals(expected.getDimensionsSpec(), actual.getDimensionsSpec());
-    Assert.assertTrue(Arrays.equals(expected.getMetricsSpec(), actual.getMetricsSpec()));
+    Assert.assertArrayEquals(expected.getMetricsSpec(), actual.getMetricsSpec());


ccaominh · 2019-10-02T01:10:01Z

+
+import java.util.Objects;
+
+public class ClientCompactionIOConfig


Do you want to add a javadoc for the class?

ccaominh · 2019-10-02T01:14:36Z

+/**
+ * Specifying an interval to compact. A hash of the segment IDs can be optionally provided for segment validation.
+ */
+public class CompactionIntervalSpec implements CompactionInputSpec


This is almost identical to ClientCompactionIntervalSpec. Is there a way to reuse the code?

They are in different modules. Added javadoc.

ccaominh · 2019-10-02T01:22:11Z

+  private final CompactionInputSpec inputSpec;
+
+  @JsonCreator
+  public CompactionIOConfig(@JsonProperty("inputSpec") CompactionInputSpec inputSpec)


Is there a serde test for this?

Yes, it's in CompactionTaskTest.

jihoonson · 2019-10-03T09:12:33Z

@ccaominh thank you for the review! Addressed most of them and left some questions.

ccaominh · 2019-10-03T18:05:03Z

 ### Compaction IOConfig

-The compaction IOConfig requires to specify `inputSpec` as seen below.
+The compaction IOConfig requires to specifying `inputSpec` as seen below.


typo: to specifying -> specifying

ccaominh · 2019-10-03T18:07:10Z

 */

-package org.apache.druid.indexer.partitions;
+package org.apache.druid.indexer;


Package for ChecksTest needs to be updated similarly

ccaominh · 2019-10-03T18:07:41Z

-class Checks
+public final class Checks
 {
+  public static <T> Property<T> checkOneNotNullOrEmpty(List<Property<T>> properties)


Please add relevant tests to ChecksTest

ccaominh

LGTM 👍

clintropolis

🤘

clintropolis · 2019-10-09T07:53:06Z

+/**
+ * InputSpec for {@link ClientCompactionIOConfig}.
+ *
+ * Should be synchronized with org.apache.druid.indexing.common.task.CompactionIntervalSpec.


clintropolis · 2019-10-09T08:12:13Z

+
+  @Nullable
+  @JsonProperty
+  public String getSha256OfSortedSegmentIds()


nit: not personally super into this name, i feel like it makes me unnecessarily care about how the segment ids were hashed, but doesn't really matter i guess since this isn't so much directly used or directly constructed by users.

jihoonson added 2 commits September 23, 2019 12:40

IOConfig for compaction task

6e507b1

add javadoc, doc, unit test

07f8fe0

jihoonson added Area - Batch Ingestion Release Notes Design Review labels Sep 23, 2019

jihoonson added 2 commits September 23, 2019 20:41

fix webconsole test

bbfdd1d

add spelling

a728ca7

ccaominh reviewed Oct 2, 2019

View reviewed changes

address comments

ed7d2f0

fix build and test

28ef88d

ccaominh reviewed Oct 3, 2019

View reviewed changes

address comments

ae89da9

ccaominh approved these changes Oct 4, 2019

View reviewed changes

clintropolis approved these changes Oct 9, 2019

View reviewed changes

gianm added this to the 0.17.0 milestone Oct 9, 2019

gianm merged commit 96d8523 into apache:master Oct 9, 2019

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

maytasm mentioned this pull request Dec 14, 2021

Support overlapping segment intervals in auto compaction #12062

Merged

9 tasks


		### Compaction IOConfig

		The compaction IOConfig requires to specify `inputSpec` as seen below.


		import java.util.Objects;

		public class ClientCompactionIOConfig

Conversation

jihoonson commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

New IOConfig for compaction task

Using interval inputSpec for auto compaction

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaominh Oct 3, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Oct 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaominh left a comment

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jihoonson commented Sep 23, 2019 •

edited

Loading

ccaominh Oct 3, 2019 •

edited

Loading