Stateful auto compaction #8573

jihoonson · 2019-09-23T08:32:43Z

Description

In addition to #8489, targetCompactionSizeBytes is dropped for the compaction task and auto compaction. targetCompactionSizeBytes was added for easy configuration, but it could be a misleading that optimizing segments should be done in terms of the size rather than number of rows. Dropping targetCompactionSizeBytes also makes things simpler such that all tasks can share the same partitionsSpec since targetCompactionSizeBytes makes sense only for compaction task.
maxRowsPerSegment now is a mandatory configuration for auto compaction. For compaction task, any partitionsSpec can be used.

Also fixed a bug that auto compaction couldn't compact an interval if there is only one segment. Note that compaction can split a segment into smaller ones.

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.

This change is

himanshug · 2019-09-25T22:54:44Z

...c/main/java/org/apache/druid/server/coordinator/helper/DruidCoordinatorSegmentCompactor.java

@@ -178,12 +178,11 @@ private CoordinatorStats doRun(
      final List<DataSegment> segmentsToCompact = iterator.next();
      final String dataSourceName = segmentsToCompact.get(0).getDataSource();

-      if (segmentsToCompact.size() > 1) {
+      if (!segmentsToCompact.isEmpty()) {


Line#179 should move in this if block.

Thank you for finding this! Will fix it.

himanshug · 2019-09-25T23:21:27Z

For compaction task, any partitionsSpec can be used.

i.e. user can explicitly run CompactionTask with any partitionSpec . Auto compaction does not take partitionsSpec as config ... right ?

jihoonson · 2019-09-27T05:45:36Z

i.e. user can explicitly run CompactionTask with any partitionSpec . Auto compaction does not take partitionsSpec as config ... right ?

For now, yes. And I'm planning to add it in the near future.

ccaominh · 2019-10-01T20:10:46Z

core/src/main/java/org/apache/druid/timeline/DataSegment.java


    @Inject(optional = true) @PruneLoadSpec boolean pruneLoadSpec = false;
+    @Inject(optional = true) @PrunePartitionsSpec boolean prunePartitionsSpec = false;


Do you want to add this to the comment on line 65?

ccaominh · 2019-10-01T20:19:47Z

core/src/main/java/org/apache/druid/timeline/DataSegment.java

@@ -68,17 +66,19 @@
   * github.com/google/guice/wiki/FrequentlyAskedQuestions#how-can-i-inject-optional-parameters-into-a-constructor
   */
  @VisibleForTesting
-  public static class PruneLoadSpecHolder
+  public static class PruneSpecs


Perhaps keep the Holder suffix as it seems to be the naming convention for the optional constructor parameter injection pattern?

ccaominh · 2019-10-01T20:22:52Z

core/src/main/java/org/apache/druid/timeline/DataSegment.java

@@ -457,6 +488,7 @@ public Builder(DataSegment segment)
      this.dimensions = segment.getDimensions();
      this.metrics = segment.getMetrics();
      this.shardSpec = segment.getShardSpec();
+      this.compactionPartitionsSpec = segment.compactionPartitionsSpec;


Why not use the getter?

ccaominh · 2019-10-01T20:25:41Z

core/src/test/java/org/apache/druid/timeline/DataSegmentTest.java

-  @Test
-  public void testBucketMonthComparator()
-  {
-    DataSegment[] sortedOrder = {
-        makeDataSegment("test1", "2011-01-01/2011-01-02", "a"),
-        makeDataSegment("test1", "2011-01-02/2011-01-03", "a"),
-        makeDataSegment("test1", "2011-01-02/2011-01-03", "b"),
-        makeDataSegment("test2", "2011-01-01/2011-01-02", "a"),
-        makeDataSegment("test2", "2011-01-02/2011-01-03", "a"),
-        makeDataSegment("test1", "2011-02-01/2011-02-02", "a"),
-        makeDataSegment("test1", "2011-02-02/2011-02-03", "a"),
-        makeDataSegment("test1", "2011-02-02/2011-02-03", "b"),
-        makeDataSegment("test2", "2011-02-01/2011-02-02", "a"),
-        makeDataSegment("test2", "2011-02-02/2011-02-03", "a"),
-    };
-
-    List<DataSegment> shuffled = new ArrayList<>(Arrays.asList(sortedOrder));
-    Collections.shuffle(shuffled);
-
-    Set<DataSegment> theSet = new TreeSet<>(DataSegment.bucketMonthComparator());
-    theSet.addAll(shuffled);
-
-    int index = 0;
-    for (DataSegment dataSegment : theSet) {
-      Assert.assertEquals(sortedOrder[index], dataSegment);
-      ++index;
-    }
-  }
-


Why is this test no longer needed?

bucketMonthComparator() is not used anywhere.

ccaominh · 2019-10-01T20:42:35Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/BatchAppenderators.java

      DataSegmentPusher segmentPusher
  )
  {
    return appenderatorsManager.createOfflineAppenderatorForTask(
        taskId,
        dataSchema,
        appenderatorConfig.withBasePersistDirectory(toolbox.getPersistDir()),
+        firehoseFactory instanceof IngestSegmentFirehoseFactory,


An alternative to using instanceof is to add another method to FirehoseFactory (i.e., polymorphism)

I know this is ugly, but I don't have a better idea. What makes most sense to me is adding a new method getTaskType(), but it's a pretty big refactoring which is not necessary in this PR.

ccaominh · 2019-10-01T21:17:38Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

+        return newTuningConfig.withPartitionsSpec(
+            new DynamicPartitionsSpec(
+                dynamicPartitionsSpec.getMaxRowsPerSegment(),
+                dynamicPartitionsSpec.getMaxTotalRowsOr(Long.MAX_VALUE)


Alternative that allocates one fewer TuningConfig for the DynamicPartitionsSpec case:

PartitionsSpec partitionsSpec = newTuningConfig.getGivenOrDefaultPartitionsSpec()); if (partitionsSpec instanceof DynamicPartitionsSpec) { final DynamicPartitionsSpec dynamicPartitionsSpec = (DynamicPartitionsSpec) partitionsSpec; partitionsSpec = new DynamicPartitionsSpec( dynamicPartitionsSpec.getMaxRowsPerSegment(), dynamicPartitionsSpec.getMaxTotalRowsOr(Long.MAX_VALUE), ); } return newTuningConfig.withPartitionsSpec(partitionsSpec);

ccaominh · 2019-10-01T21:20:20Z

indexing-service/src/test/java/org/apache/druid/indexing/common/task/CompactionTaskRunTest.java

@@ -191,6 +192,10 @@ public void testRun() throws Exception
          Intervals.of("2014-01-01T0%d:00:00/2014-01-01T0%d:00:00", i, i + 1),
          segments.get(i).getInterval()
      );
+      Assert.assertEquals(
+          new DynamicPartitionsSpec(5000000, Long.MAX_VALUE),


Perhaps save this as a named constant since it's used a lot?

ccaominh · 2019-10-01T21:26:10Z

server/src/main/java/org/apache/druid/server/coordinator/helper/NewestSegmentFirstIterator.java

@@ -201,7 +208,7 @@ private void updateQueue(String dataSourceName, DataSourceCompactionConfig confi
              .filter(holder -> {
                final List<PartitionChunk<DataSegment>> chunks = Lists.newArrayList(holder.getObject().iterator());
                final long partitionBytes = chunks.stream().mapToLong(chunk -> chunk.getObject().getSize()).sum();
-                return chunks.size() > 1
+                return chunks.size() > 0


Prefer !chunks.isEmpty() (similar to the change you made on line 185)

ccaominh · 2019-10-01T21:33:09Z

server/src/main/java/org/apache/druid/server/coordinator/helper/NewestSegmentFirstIterator.java

-              candidates.segments.get(0).getDataSource(),
-              candidates.segments.get(0).getInterval()
-          );
+      if (candidates.getNumSegments() > 0) {


Prefer !candidates.isEmpty()

ccaominh · 2019-10-01T21:52:04Z

server/src/main/java/org/apache/druid/server/coordinator/helper/NewestSegmentFirstIterator.java

@@ -229,15 +236,58 @@ public boolean hasNext()
    }
  }

+  private static boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCompact candidates)


Which tests cover this new logic?

DruidCoordinatorSegmentCompactorTest verifies the behavior of DruidCoordinatorSegmentCompactor.

…ful-compaction-master

jihoonson · 2019-10-10T23:35:48Z

I also changed dataSegment to remember lastCompactionState including partitionsSpec and indexSpec per discussion at #8489 (comment).

lgtm-com · 2019-10-11T01:43:28Z

This pull request introduces 1 alert when merging 47dca61 into 6c60929 - view on LGTM.com

new alerts:

1 for Self assignment

himanshug · 2019-10-11T20:28:02Z

core/src/main/java/org/apache/druid/timeline/CompactionState.java

+import java.util.Map;
+import java.util.Objects;
+
+public class CompactionState


Can you please add javadoc for this class describing what this is, why it has the field it does ...(I know there is some discussion in proposal, but it would be very non-obvious for someone reading the code) and what guarantees it provides ... e.g. something like if a CompactionTask is run with parameters matching here then row distribution in segments created would be exactly same.

Added javadoc. Please take a look if it's enough.

LGTM, thanks.

himanshug · 2019-10-11T20:31:48Z

core/src/main/java/org/apache/druid/timeline/CompactionState.java

+public class CompactionState
+{
+  private final PartitionsSpec partitionsSpec;
+  // org.apache.druid.segment.IndexSpec cannot be used here to avoid the dependency cycle


couldn't understand what is the cycle ? .. do you mean IndexSpec can contain a CompactionState , so json serde would fail ?

The thing is IndexSpec is in the processing module while this class is in the core module. Since processing has a dependency on core, I cannot add a new dependency of core -> processing since it will introduce a cycle. I updated this comment more understandable.

got it, thanks.

maybe in next round of module merge: merge core into processing if there is no use case of anyone depending on druid-core directly :)

himanshug · 2019-10-11T20:33:36Z

docs/configuration/index.md

@@ -786,8 +786,7 @@ A description of the compaction config is:
 |`dataSource`|dataSource name to be compacted.|yes|
 |`taskPriority`|[Priority](../ingestion/tasks.html#priority) of compaction task.|no (default = 25)|
 |`inputSegmentSizeBytes`|Maximum number of total segment bytes processed per compaction task. Since a time chunk must be processed in its entirety, if the segments for a particular time chunk have a total size in bytes greater than this parameter, compaction will not run for that time chunk. Because each compaction task runs with a single thread, setting this value too far above 1–2GB will result in compaction tasks taking an excessive amount of time.|no (default = 419430400)|
-|`targetCompactionSizeBytes`|The target segment size, for each segment, after compaction. The actual sizes of compacted segments might be slightly larger or smaller than this value. Each compaction task may generate more than one output segment, and it will try to keep each output segment close to this configured size. This configuration cannot be used together with `maxRowsPerSegment`.|no (default = 419430400)|


we should probably add a blurb in release notes for this , just in case some people set this property and expect something to happen.

himanshug · 2019-10-11T20:44:56Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/BatchAppenderators.java

+      isReingest = dataSchema.getDataSource().equals(((IngestSegmentFirehoseFactory) firehoseFactory).getDataSource());
+    } else {
+      isReingest = false;
+    }


is it possible to drive this from auto compaction code in druid coordinator instead as IngestSegmentFirehoseFactory could be used outside of auto compaction as well. For example, as a user , knowing my data flow, I can setup re-index task to run every day for previous day's data .. sort of a manual compaction. But in that case, the CompactionState doesn't need to be preserved.

Hmm, I thought lastCompactionState could be useful even for manual compaction as well. How about adding a parameter to taskContext to store lastCompactionState, so that other users also can use it if they want?

yeah , that would work and let user (in this case auto compaction code) explicitly say whether CompactionState should be saved or not.

Thanks, added a new task context configuration. I'm still not sure whether this should be documented though.

For now, I think this config is best left undocumented and for auto compaction internal usage only.

jihoonson · 2019-10-14T22:53:39Z

Some docs should be further updated, but I will do it once this PR and #8570 are merged.

jihoonson · 2019-10-16T16:09:52Z

Thanks for the review @ccaominh and @himanshug!

jihoonson added 3 commits September 23, 2019 16:33

Stateful auto compaction

93b10d0

javaodc

8e7d326

add removed test back

b511b5b

jihoonson added the Release Notes label Sep 23, 2019

fix test

b2d0489

himanshug reviewed Sep 25, 2019

View reviewed changes

ccaominh reviewed Oct 1, 2019

View reviewed changes

jihoonson added 6 commits October 10, 2019 12:14

adding indexSpec to compactionState

8958062

fix build

33e20a7

add lastCompactionState

4fde410

Merge branch 'master' of github.com:apache/incubator-druid into state…

e7a7994

…ful-compaction-master

address comments

c9bfed9

extract CompactionState

8fe8297

fix doc

47dca61

himanshug reviewed Oct 11, 2019

View reviewed changes

jihoonson added 2 commits October 11, 2019 16:27

fix build and test

8000b41

Add a task context to store compaction state; add javadoc

47df972

himanshug approved these changes Oct 15, 2019

View reviewed changes

fix it test

fba9e04

jihoonson merged commit 4046c86 into apache:master Oct 16, 2019

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

jihoonson mentioned this pull request Apr 7, 2020

Auto compaction can get stuck #8481

Closed

jihoonson mentioned this pull request Apr 24, 2020

Enable auto minor compaction #9712

Closed


		@Inject(optional = true) @PruneLoadSpec boolean pruneLoadSpec = false;
		@Inject(optional = true) @PrunePartitionsSpec boolean prunePartitionsSpec = false;

Stateful auto compaction #8573

Stateful auto compaction #8573

Conversation

jihoonson commented Sep 23, 2019 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Sep 25, 2019

jihoonson commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Oct 10, 2019

lgtm-com bot commented Oct 11, 2019

himanshug Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Oct 14, 2019

jihoonson commented Oct 16, 2019

jihoonson commented Sep 23, 2019 •

edited

Loading

himanshug Oct 11, 2019 •

edited

Loading

himanshug Oct 15, 2019 •

edited

Loading