Add support minor compaction with segment locking by jihoonson · Pull Request #7547 · apache/druid

jihoonson · 2019-04-25T00:49:08Z

Apologize for the huge PR. I tried to split into sub PRs, but I think it's not easy split more because new changes for segment locking are tightly coupled together and it won't be easy to understand if they are in separate PRs.

This PR fixes #7491 and now it's ready for review.

Here are the key classes to be reviewed.

Overshadowable

New interface to represent a class which can have overshadow relation between its instances. Ex) dataSegment

ShardSpecFactory

New interface to be used to allocate segments remotely in the overlord.

NumberedOverwriteShardSpec

New shardSpec for segments which may overshadow others with their minor version.

DataSegment

getVersion is renamed to getMajorVersion.
Some short circuit methods to check overshadow relation between segments.

VersionedIntervalTimeline

Improved to consider the new overshadow relation properly.

OvershadowableManager

A new class manages the state of AtomicUpdateGroup.

AtomicUpdateGruop

A set of PartitionChunks which should be atomically visible or not in the timeline.

SegmentTransactionalInsertAction

Added a new parameter segmentsToBeOverwritten. Shouldn't be empty if the new segments are created with segment locks.

AbstractBatchIndexTask

Abstract class for batch tasks like indexTask or parallelIndexTask. Provides some methods for easily acquiring task locks.

IndexTaskSegmentAllocator

Segment allocator interface for only IndexTask. It has 3 different modes for allocating segments.

The new segment locking is currently available for only Kafka/Kinesis index task, index task, and parallel index task.

This PR is marked as Incompatible because it introduces a new shardSpec. It's not possible to roll back after this PR. For rolling update while kafka/kinesis indexing service is running, any kind of overwriting tasks (including compaction task and index task) should be stopped first. Otherwise, kafka/kinesis index tasks might fail when they list segments with the new shardSpec generated by overwriting tasks. Native parallel index tasks cannot be run in middleManagers of mixed versions, so they should be stopped first before update.

Currently there is a known issue of potential race in task locking. If two or more overwriting tasks try to acquire segment locks for overlapped sets of segments, each task acquires locks one by one per segment in the current implementation. This may lead to failures of both tasks if they hold locks necessary for each other. However, I think this happens rarely and so will fix in a follow-up PR.

I've been testing this PR in our in-house cluster since I raised this PR and it looks fine so far.

jon-wei · 2019-07-24T01:05:24Z

+
+/**
+ * Abstract class for batch tasks like {@link IndexTask}.
+ * Provides some methods ({@link #determineSegmentGranularity} and {@link #determineSegmentGranularity}) for easily acquiring task


determineSegmentGranularity is mentioned twice here

Thanks, fixed javadoc.

jon-wei · 2019-07-24T01:30:30Z

+  {
+    if (requireLockExistingSegments()) {
+      if (isPerfectRollup()) {
+        log.info("Using timeChunk lock for perfrect rollup");


perfrect -> perfect

Thanks, fixed.

jon-wei · 2019-07-24T01:31:28Z


-  @Nullable
-  public OverwritingRootGenerationPartitions getOverwritingSegmentMeta(Interval interval)
+  private static class LockGranularityDeterminResult


LockGranularityDeterminResult -> LockGranularityDetermineResult

Thanks, fixed.

jon-wei · 2019-07-24T01:32:21Z

+
+      for (SegmentIdWithShardSpec segmentIdentifier : idsPerInterval) {
+        shardSpecMap.computeIfAbsent(interval, k -> new ArrayList<>()).add(segmentIdentifier.getShardSpec());
+        // The shardSpecs for partitinoing and publishing can be different if isExtendableShardSpecs = true.


partitinoing -> partitioning

Thanks, fixed.

jon-wei · 2019-07-24T03:39:59Z

    if (bucketIntervals.isPresent()) {
-      // If the granularity spec has explicit intervals, we just need to find the interval (of the segment
-      // granularity); we already tried to lock it at task startup.
+      // If granularity spec has explicit intervals, we just need to find the version accociated to the interval.


accociated -> associated

Thanks, fixed.

jon-wei

LGTM

clintropolis · 2019-07-24T07:01:44Z

+  /**
+   * Returns true if this segment overshadows the other segment.
+   */
+  default boolean isOvershadow(T other)


I think the purpose of this method would be clearer if it was called something like overshadows or doesOvershadow or willOvershadow. It reads with the intention x.overshadows(y) of x.willOvershadow(y) is very clear that it's a check if y will be overshadowed by x.

I think isOvershadow it too close to x is overshadowed by y, which is why I keep having to check the javadoc to remember which one is the overshadowed segment, and will also help it standout/distinguish it from the timeline isOvershadowed method.

Renamed to overshadows().

clintropolis · 2019-07-24T07:43:22Z

+      final Set<DataSegment> inputSegments = isUseSegmentLock()
+                                             ? getSegmentLockHelper().getLockedExistingSegments()
+                                             : null;
+      final SegmentsAndMetadata published = awaitPublish(driver.publishAll(inputSegments, publisher), pushTimeout);


is driver.publishAll going to be sad if inputSegments is null because not using segment lock? could you explain this a bit better in comments?

Added a comment.

clintropolis · 2019-07-24T08:03:34Z

+
+  private boolean tryTimeChunkLock(TaskActionClient client, List<Interval> intervals) throws IOException
+  {
+    // In this case, the intervals to lock must be alighed with segmentGranularity if it's defined


typo:

... must be aligned with ...

Thanks, fixed.

clintropolis · 2019-07-24T20:16:00Z

+    }
+
+    final List<Short2ObjectMap.Entry<AtomicUpdateGroup<T>>> found = new ArrayList<>();
+    while (current != null && rangeOfAug.contains(current.getKey())) {


this method could maybe use a few more comments to make understanding what is going on a bit easier

👍 added comments and javadoc.

clintropolis

lgtm, 👍

jihoonson · 2019-07-25T00:35:59Z

@jon-wei @clintropolis thank you for the review!!

wengwh · 2020-06-05T03:16:31Z

before the version，we use hadoop-task and kafka-task ingestion data into the same datasource, is success.
but upgrade new version, if one segment is ingestion by hadoop task,the shardSpec will be hashed,
the kafka task ingestion the same segment will exception
org.apache.druid.java.util.common.ISE: Could not allocate segment for row with timestamp

now we must run compact task to solve the exception

Is there any new solution to this problem？

* Segment locking * Allow both timeChunk and segment lock in the same gruop * fix it test * Fix adding same chunk to atomicUpdateGroup * resolving todos * Fix segments to lock * fix segments to lock * fix kill task * resolving todos * resolving todos * fix teamcity * remove unused class * fix single map * resolving todos * fix build * fix SQLMetadataSegmentManager * fix findInputSegments * adding more tests * fixing task lock checks * add SegmentTransactionalOverwriteAction * changing publisher * fixing something * fix for perfect rollup * fix test * adjust package-lock.json * fix test * fix style * adding javadocs * remove unused classes * add more javadocs * unused import * fix test * fix test * Support forceTimeChunk context and force timeChunk lock for parallel index task if intervals are missing * fix travis * fix travis * unused import * spotbug * revert getMaxVersion * address comments * fix tc * add missing error handling * fix backward compatibility * unused import * Fix perf of versionedIntervalTimeline * fix timeline * fix tc * remove remaining todos * add comment for parallel index * fix javadoc and typos * typo * address comments

jihoonson added 10 commits April 22, 2019 15:51

Segment locking

17a1158

Allow both timeChunk and segment lock in the same gruop

9f24604

fix it test

e47dcdc

Fix adding same chunk to atomicUpdateGroup

16b56cf

resolving todos

a915a63

Fix segments to lock

30dc9b9

fix segments to lock

3378f1a

fix kill task

95b4e09

resolving todos

3f128e4

resolving todos

a297085

jihoonson added Area - Batch Ingestion Incompatible Release Notes WIP Area - Streaming Ingestion labels Apr 25, 2019

jihoonson changed the title ~~Minor compaction~~ Add support minor compaction with segment locking Apr 25, 2019

jihoonson added 3 commits April 24, 2019 22:59

fix teamcity

6a72b47

remove unused class

6c5d477

fix single map

fda329a

jon-wei mentioned this pull request Apr 25, 2019

Add reload by interval API #7490

Merged

jihoonson added 10 commits April 26, 2019 16:24

resolving todos

c8a2cf5

Merge branch 'master' of github.com:druid-io/druid into minor-compaction

2cf3f3d

fix build

7140f0c

Merge branch 'master' of github.com:druid-io/druid into minor-compaction

5b9c78d

fix SQLMetadataSegmentManager

7a47112

fix findInputSegments

2305a46

adding more tests

28890b7

fixing task lock checks

af4697f

add SegmentTransactionalOverwriteAction

948d335

changing publisher

f0d2391

jihoonson added 7 commits July 18, 2019 00:27

fix timeline

e445db4

Merge branch 'master' of github.com:druid-io/druid into minor-compaction

8eafbb3

fix tc

dfe7985

Merge branch 'master' of github.com:druid-io/druid into minor-compaction

d2c69b1

Merge branch 'master' of github.com:druid-io/druid into minor-compaction

f628d3b

remove remaining todos

920376b

add comment for parallel index

b3b05db

jon-wei reviewed Jul 24, 2019

View reviewed changes

fix javadoc and typos

610745e

jon-wei reviewed Jul 24, 2019

View reviewed changes

jon-wei approved these changes Jul 24, 2019

View reviewed changes

typo

cb2de9a

clintropolis reviewed Jul 24, 2019

View reviewed changes

address comments

c1d1bf7

clintropolis approved these changes Jul 24, 2019

View reviewed changes

jihoonson merged commit db14946 into apache:master Jul 25, 2019

jihoonson mentioned this pull request Jul 25, 2019

Add benchmark for VersionedIntervalTimeline #8161

Merged

1 task

leventov mentioned this pull request Jul 26, 2019

Clarifying VersionedIntervalTimeline.findFullyOvershadowed() #8167

Open

clintropolis added this to the 0.16.0 milestone Aug 8, 2019

clintropolis mentioned this pull request Aug 21, 2019

0.16.0-incubating release notes #8369

Closed

jihoonson mentioned this pull request Sep 26, 2019

Stateful auto compaction #8489

Closed

jihoonson mentioned this pull request Oct 11, 2019

Kafka indexing service duplicate entry exception in druid_pendingSegments #8663

Closed

capistrant mentioned this pull request Nov 15, 2019

Add override for new method in Granularity class Zenysis/druid-arbitrary-granularity#2

Merged

jihoonson mentioned this pull request Feb 11, 2020

Broken feature: appending linearly partitioned segments into a hash partitioned datasource #9352

Closed

wengwh mentioned this pull request Jun 5, 2020

org.apache.druid.java.util.common.ISE: Could not allocate segment for row with timestamp #9990

Open

jihoonson mentioned this pull request Jun 25, 2020

Allow append to existing datasources when dynamic partitioning is used #10033

Merged

9 tasks

GWphua mentioned this pull request Feb 25, 2026

feat: Native Minor compaction #19016

Merged

9 tasks

Uh oh!

Conversation

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

Uh oh!

Overshadowable

ShardSpecFactory

NumberedOverwriteShardSpec

DataSegment

VersionedIntervalTimeline

OvershadowableManager

AtomicUpdateGruop

SegmentTransactionalInsertAction

AbstractBatchIndexTask

IndexTaskSegmentAllocator

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!

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

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!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Jul 25, 2019

Uh oh!

wengwh commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jihoonson commented Apr 25, 2019 •

edited

Loading