Fix auto compaction by adjusting compaction task's interval to align with segmentGranularity when segmentGranularity is set by maytasm · Pull Request #12334 · apache/druid

maytasm · 2022-03-15T06:10:58Z

Fix auto compaction by adjusting compaction task's interval to align with segmentGranularity when segmentGranularity is set

Description

This bug can cause data lost if segmentGranularity is set in auto compaction.

If segmentGranularity is set, then the intervals of the segments to be compacted may not align with the configured segmentGranularity. We must adjust the interval of the compaction task to fully cover and align with the segmentGranularity to prevent unexpected data lost.

For example,

The umbrella interval of the segments to be compacted is 2015-04-11/2015-04-12 but configured segmentGranularity is YEAR, if the compaction task's interval is 2015-04-11/2015-04-12 then we can run into race condition where after submitting the compaction task if a new segment outside of the interval (i.e. 2015-02-11/2015-02-12) got created will be lost as it is overshadowed by the compacted segment (compacted segment has interval 2015-01-01/2016-01-01). Hence, in this case, we must adjust the compaction task interval to 2015-01-01/2016-01-01.
The umbrella interval of the segments to be compacted is 2015-02-01/2015-03-01 but configured segmentGranularity is WEEK, if the compaction task's interval is 2015-02-01/2015-03-01 then compacted segments created will be 2015-01-26/2015-02-02, 2015-02-02/2015-02-09, 2015-02-09/2015-02-16, 2015-02-16/2015-02-23, 2015-02-23/2015-03-02. The compacted segment would cause existing data from 2015-01-26 to 2015-02-01 and 2015-03-01 to 2015-03-02 to be lost. Hence, in this case, we must adjust the compaction task interval to 2015-01-26/2015-03-02

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.

jihoonson · 2022-03-16T02:25:03Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionIntervalSpec.java

  private final String sha256OfSortedSegmentIds;

-  public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments)
+  public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments, Granularity segmentGranularity)


Suggested change

public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments, Granularity segmentGranularity)

public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments, @Nullable Granularity segmentGranularity)

jihoonson · 2022-03-16T02:32:55Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionIntervalSpec.java

+      // - The umbrella interval of the segments is 2015-02-01/2015-03-01 but configured segmentGranularity is MONTH,
+      // if the compaction task's interval is 2015-02-01/2015-03-01 then compacted segments created will be
+      // 2015-01-26/2015-02-02, 2015-02-02/2015-02-09, 2015-02-09/2015-02-16, 2015-02-16/2015-02-23, 2015-02-23/2015-03-02.


I'm not sure how the segments will have these intervals after compacted. Can you elaborate? It would be nice to add your explanation in the comment.

oops typo. The configured segmentGranularity in auto compaction is WEEK. Added more details to the comment too.

jihoonson · 2022-03-16T02:39:26Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionIntervalSpec.java

+      // 2015-01-26/2015-02-02, 2015-02-02/2015-02-09, 2015-02-09/2015-02-16, 2015-02-16/2015-02-23, 2015-02-23/2015-03-02.
+      // The compacted segment would cause existing data from 2015-01-26 to 2015-02-01 and 2015-03-01 to 2015-03-02 to be lost.
+      //  Hence, in this case, we must adjust the compaction task interval to 2015-01-26/2015-03-02
+      interval = JodaUtils.umbrellaInterval(segmentGranularity.getIterable(interval));


Looking at JodaUtils.umbrellaInterval(), I think this code can cause OOM if segmentGranularity.getIterable() returns lots of intervals. JodaUtils.umbrellaInterval() doesn't seem to need to store all startDates and endDates in memory. Instead, it can keep only one pair of minStartDate and maxEndDate which are updated in the loop. Should we fix it in this PR too?

Or should we have a guardrail for segmentGranularity to not return such many intervals?

I don't think we need a guardrail here. The list of segments passed to this method should be from a single time chunk (from the CompactionSegmentIterator). The single time chuck is then adjusted based on some segmentGranularity. Since the original interval (from the umbrellaInterval of the list of segments) is limited to a single time chunk, segmentGranularity.getIterable() should not returns lots of intervals. The edge case to this is if the granularities are either ALL or NONE. I think we should put a guardrail against ALL or NONE but not here. (actually I think it would blow up in the NewestSegmentFirstIterator before getting here anyway)

I agree that the guardrail should be somewhere else than here.

jihoonson · 2022-03-16T02:51:30Z

...ration-tests/src/test/java/org/apache/druid/tests/coordinator/duty/ITAutoCompactionTest.java

+  }
+
+  @Test
+  public void testAutoCompactionDutyWithSegmentGranularityCoarserAndNotAlignWithSegment() throws Exception


Should this tests week -> month instead of month -> year if it's for the case when the new granularity does not align with the previous?

yuanlihan · 2022-03-16T05:18:08Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionIntervalSpec.java

+      // from 2015-01-26 to 2015-02-01 and 2015-03-01 to 2015-03-02 to be lost. Hence, in this case,
+      // we must adjust the compaction task interval to 2015-01-26/2015-03-02
+      interval = JodaUtils.umbrellaInterval(segmentGranularity.getIterable(interval));
+    }


Is it necessary to log some message about the interval extension? If the interval was extended, extra segments may be included in this compaction task and then the total input bytes may exceed inputSegmentSizeBytes.

Added some extra logs.
Regarding inputSegmentSizeBytes, I think inputSegmentSizeBytes should actually be deprecated now that the issued compaction task can run in parallel.

jihoonson

LGTM overall. Please check the CI failure. It seems legit.

jihoonson · 2022-03-16T06:51:23Z

core/src/main/java/org/apache/druid/java/util/common/JodaUtils.java

-    DateTime minStart = minDateTime(startDates.toArray(new DateTime[0]));
-    DateTime maxEnd = maxDateTime(endDates.toArray(new DateTime[0]));
-
    if (minStart == null || maxEnd == null) {


This function now will return an interval of Long.MIN_VALUE, Long.MAX_VALUE when the input intervals is empty. It should throw an exception instead.

BTW, I'm OK if you don't want to make this change in this PR. It's optional and up to you.

Oops. Fixed.

jihoonson · 2022-03-16T06:57:14Z

server/src/main/java/org/apache/druid/client/indexing/ClientCompactionIntervalSpec.java

+      // 2015-01-26/2015-02-02, 2015-02-02/2015-02-09, 2015-02-09/2015-02-16, 2015-02-16/2015-02-23, 2015-02-23/2015-03-02.
+      // The compacted segment would cause existing data from 2015-01-26 to 2015-02-01 and 2015-03-01 to 2015-03-02 to be lost.
+      //  Hence, in this case, we must adjust the compaction task interval to 2015-01-26/2015-03-02
+      interval = JodaUtils.umbrellaInterval(segmentGranularity.getIterable(interval));


I agree that the guardrail should be somewhere else than here.

jihoonson

LGTM. Thanks @maytasm!

…with segmentGranularity when segmentGranularity is set (apache#12334) * add impl * add ITs * address comments * address comments * address comments * fix failure * fix checkstyle * fix checkstyle

maytasm added 2 commits March 14, 2022 23:07

add impl

c984ff7

add ITs

69bb86b

maytasm changed the title ~~Fix auto compaction task interval when segmentGranularity is set~~ Fix auto compaction by adjusting compaction task's interval to align with segmentGranularity when segmentGranularity is set Mar 15, 2022

jihoonson added Bug Area - Compaction labels Mar 16, 2022

jihoonson reviewed Mar 16, 2022

View reviewed changes

maytasm added 2 commits March 15, 2022 21:38

address comments

f053c1b

address comments

61a0bfd

yuanlihan reviewed Mar 16, 2022

View reviewed changes

address comments

3bd994f

jihoonson reviewed Mar 16, 2022

View reviewed changes

fix failure

d6bcd69

jihoonson approved these changes Mar 16, 2022

View reviewed changes

maytasm added 2 commits March 16, 2022 12:54

fix checkstyle

4156b31

fix checkstyle

c41154c

maytasm merged commit dbb9518 into apache:master Mar 18, 2022

maytasm deleted the IMPLY-16895 branch March 18, 2022 19:46

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

	public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments, Granularity segmentGranularity)
	public static ClientCompactionIntervalSpec fromSegments(List<DataSegment> segments, @Nullable Granularity segmentGranularity)

Conversation

maytasm commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maytasm commented Mar 15, 2022 •

edited

Loading