Optimize isOvershadowed when there is a unique minor version for an interval #15952

AmatyaAvadhanula · 2024-02-23T04:49:56Z

The coordinator tries to poll segments every minute (or based on the configured periodicPollDelay)
In each cycle, the DataSourcesSnapshot

Polls the database for all used segments
Create a segment timeline for each of the datasources
Determine all the overshadowed segments

The third step to compute overshadowed segments is currently based on calling isOvershadowed on each segment individually. The complexity for this operation is O(n ^ 2) for each interval with n segments.
This can lead to coordinator polls happening infrequently, which causes a delay in segments being discovered after having been committed, which leads to stale data and slow handoffs etc.

Optimize the isOvershadowed method when there there is a single value of minorVersion across all segments in an interval (When segment locking is not used, for example) to reduce the complexity to O(n) in such cases.
This is a simple optimization that may be useful for most large clusters since segment locking is seldom used.

BENCHMARKS:
VersionedIntervalTimelineBenchmark#benchIsOvershadowedTotal
Benchmarks were run with 1000 / 10000 segments per interval with Monthly granularity over a year.

Existing code:

Benchmark                                                    (numInitialRootGenSegmentsPerInterval)  (numNonRootGenerations)  (segmentGranularity)  (useSegmentLock)   Mode  Cnt  Score    Error  Units
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        1                 MONTH             false  thrpt    3  0.347 ±  0.030  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        1                 MONTH              true  thrpt    3  0.366 ±  0.033  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        2                 MONTH             false  thrpt    3  0.343 ±  0.015  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        2                 MONTH              true  thrpt    3  0.365 ±  0.033  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        1                 MONTH             false  thrpt    3  0.004 ±  0.001  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        1                 MONTH              true  thrpt    3  0.004 ±  0.001  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        2                 MONTH             false  thrpt    3  0.003 ±  0.001  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        2                 MONTH              true  thrpt    3  0.004 ±  0.001  ops/s

With this patch:

Benchmark                                                    (numInitialRootGenSegmentsPerInterval)  (numNonRootGenerations)  (segmentGranularity)  (useSegmentLock)   Mode  Cnt   Score    Error  Units
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        1                 MONTH             false  thrpt    3  82.062 ±  5.082  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        1                 MONTH              true  thrpt    3  80.937 ± 33.370  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        2                 MONTH             false  thrpt    3  59.582 ±  6.060  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                    1000                        2                 MONTH              true  thrpt    3  59.314 ±  9.134  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        1                 MONTH             false  thrpt    3   8.165 ±  0.675  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        1                 MONTH              true  thrpt    3   8.193 ±  0.431  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        2                 MONTH             false  thrpt    3   5.831 ±  1.910  ops/s
VersionedIntervalTimelineBenchmark.benchIsOvershadowedTotal                                   10000                        2                 MONTH              true  thrpt    3   5.927 ±  0.421  ops/s

TODO:

Add tests to ensure correctness of the new method

This PR has:

…dowed

server/src/main/java/org/apache/druid/metadata/SegmentsMetadataManagerConfig.java

…ment_snapshots

kfaraz

@AmatyaAvadhanula , the updated logic looks pretty neat! left some minor comments regarding logs.

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java

…ment_snapshots

kfaraz · 2024-03-01T07:00:40Z

@AmatyaAvadhanula , looking at the benchmarking, is it correct to conclude that the new logic is faster even when useSegmentLock = true (i.e. minor versions exist)? But how can that be the case?
Is it because the object.getMinorVersion turns out to be equal to the maxMinorVersion?

So the performance would be worst only when we have multiple minor versions?

Another point, is the default minor version of -1 safe?

What is the default value of minor version for any segment when it actually has no minor version? I should assume that the value would be 0.

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

kfaraz · 2024-03-01T09:40:48Z

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

  }

  public boolean add(PartitionChunk<T> chunk)
  {
-    return overshadowableManager.addChunk(chunk);
+    boolean added = overshadowableManager.addChunk(chunk);


Don't we need to do anything on remove()?

If we do, we would have to maintain counts and complicate the logic.
This patch is intended to help large clusters without segment locking i.e distinct minor versions.
Would it be better if a more elaborate solution which updates the state when remove is called is implemented?

Yes, we would have to maintain counts for each minor version.

So if we skip the impl on remove, if a minor version was ever added to the timeline, we will continue to assume that that minor version is still present for the purposes of computing maxMinorVersion. So we might sometimes enter into the slow code flow even though it could have been avoided. I guess that's not so bad.

For the timeline in the DataSourcesSnapshot, the impl in remove doesn't matter (at least in the current implementation of the SqlSegmentsMetadataManager) since objects are never removed, only added to the timeline.

I guess we can keep the impl simple for now, as you suggest, but we should atleast call it out in a comment.

Also, since the logic of the maxMinorVersion is very adhoc and confusing, we should add comments as appropriate and keep the changes limited to the VersionedIntervalTimeline class.

…ment_snapshots

kfaraz

Left some final comments.

kfaraz · 2024-03-20T06:52:51Z

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

@@ -34,16 +34,26 @@ public class PartitionHolder<T extends Overshadowable<T>> implements Iterable<Pa
 {
  private final OvershadowableManager<T> overshadowableManager;

+  // Maintains the maximum minor version across all the added segments.


This comment should instead be a javadoc in the getMinorVersion method.

kfaraz · 2024-03-20T06:56:35Z

processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java

+  /**
+   * 0) Determine the TimelineEntry with the given interval
+   * 1) If the major versions are equal, the segment is overshadowed iff any of the segments in the TimelineEntry overshadows it.
+   * 2) If the major versions are different, simply return true if the TimelineEntry has a greater major version. False otherwise.
+   * 3) If there is no TimelineEntry with the same interval, find one which contains the input interval.
+   * 4) If a TimelineEntry is found in step 3, repeat steps 1, 2 with it. Return false otherwise.
+   *
+   * @param interval The interval of the overshadowable object (segment)
+   * @param version The version of the overshadowable object (segment)
+   * @param object The overshadowable object to be determined as overshadowed or not
+   * @return true iff the object is overshadowed in this timeline
+   */


This javadoc seems more like an implementation detail rather than the overall behaviour of this method. I don't think this is needed.

kfaraz · 2024-03-20T06:57:18Z

processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java

@@ -815,6 +832,11 @@ public PartitionHolder<ObjectType> getPartitionHolder()
      return partitionHolder;
    }

+    public short getMaxMinorVersion()


Does this need to be public?

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

kfaraz · 2024-03-20T06:59:33Z

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java

@@ -1063,12 +1063,20 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE
    if (segments.isEmpty()) {
      log.info("No segments found in the database!");
    } else {
-      log.info("Polled and found %,d segments in the database", segments.size());
+      log.info("Polled and found %,d segments in the database.", segments.size());


Suggested change

log.info("Polled and found %,d segments in the database.", segments.size());

log.info("Polled and found [%,d] segments in the database.", segments.size());

kfaraz · 2024-03-20T07:00:45Z

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java

    dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(
        Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method).
        dataSourceProperties
    );
+    final long snapshotCreationEndTime = System.currentTimeMillis();


Instead of this, initialize a Druid Stopwatch at the start of this method. And in every log, print the result of .millisElapsed() to get a clear idea of the total time spent after performing each step.

Thanks! Done.

…ment_snapshots

kfaraz · 2024-03-20T11:25:00Z

processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java

+     * Returns the maximum minor version across all the added segments.
+     * We do not handle updates of this variable when segments are removed for the sake of simplicity.
+     */
+    short getMaxMinorVersion()


Nit: This method should be private.

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java

kfaraz · 2024-03-20T11:25:31Z

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java

@@ -1014,7 +1015,8 @@ public void poll()
  @GuardedBy("pollLock")
  private void doPoll()
  {
-    log.debug("Starting polling of segment table");
+    Stopwatch stopwatch = Stopwatch.createStarted();


Suggested change

Stopwatch stopwatch = Stopwatch.createStarted();

final Stopwatch stopwatch = Stopwatch.createStarted();

Done, thanks

kfaraz · 2024-03-20T11:26:06Z

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java

    }
+    stopwatch.restart();
+
+    log.debug("Building datasources snapshot from polled segments.");


This log line is not needed.

kfaraz

some minor comments, otherwise changes look good.

AmatyaAvadhanula · 2024-03-20T14:00:06Z

Thank you for the review @kfaraz!

maytasm · 2024-05-22T22:38:24Z

On a cluster with 600k active segments, this patch reduce the time to build timeline from 160,000ms to 2,000ms.
On a cluster with ~7 million active segments, this patch reduce the time to build timeline from 850,000ms to 35,000ms. We have some datasources with 100K++ segments.

Add config to replace individual isOvershadowed with findFullyOversha…

ef8248c

…dowed

kfaraz reviewed Feb 25, 2024

View reviewed changes

server/src/main/java/org/apache/druid/metadata/SegmentsMetadataManagerConfig.java Outdated Show resolved Hide resolved

AmatyaAvadhanula added 4 commits February 28, 2024 00:07

Optimize isOvershadowed for intervals with timechunk locking

ff9bcd7

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

0506eef

…ment_snapshots

Clean up

37c3f58

Add benchmark

ff31927

AmatyaAvadhanula changed the title ~~Replace individual isOvershadowed with findFullyOvershadowed~~ Optimize isOvershadowed when there is a unique minor version for an interval Feb 27, 2024

kfaraz marked this pull request as ready for review March 1, 2024 06:38

kfaraz approved these changes Mar 1, 2024

View reviewed changes

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

2b1a74e

…ment_snapshots

Update maxMinorVersion when new segments are added

95e022e

kfaraz reviewed Mar 1, 2024

View reviewed changes

kfaraz self-requested a review March 1, 2024 09:40

AmatyaAvadhanula added 6 commits March 4, 2024 09:22

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

cd35a2d

…ment_snapshots

Clean up

c5dc869

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

cae4d92

…ment_snapshots

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

dd378ad

…ment_snapshots

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

dc2a1b0

…ment_snapshots

Add logs and docs

06b362d

AmatyaAvadhanula mentioned this pull request Mar 15, 2024

Introduce Segment Schema Publishing and Polling for Efficient Datasource Schema Building #15817

Merged

10 tasks

kfaraz reviewed Mar 20, 2024

View reviewed changes

AmatyaAvadhanula added 2 commits March 20, 2024 12:33

Merge remote-tracking branch 'upstream/master' into more_frequent_seg…

c446053

…ment_snapshots

Use stopwatch and clean up

8e99fc9

kfaraz self-requested a review March 20, 2024 11:22

kfaraz reviewed Mar 20, 2024

View reviewed changes

processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java Show resolved Hide resolved

kfaraz reviewed Mar 20, 2024

View reviewed changes

kfaraz approved these changes Mar 20, 2024

View reviewed changes

Address feedback

dc486ac

AmatyaAvadhanula merged commit 488d376 into apache:master Mar 20, 2024
85 checks passed

adarshsanjeev added this to the 30.0.0 milestone May 6, 2024

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize isOvershadowed when there is a unique minor version for an interval #15952

Optimize isOvershadowed when there is a unique minor version for an interval #15952

AmatyaAvadhanula commented Feb 23, 2024 •

edited

Loading

kfaraz left a comment

kfaraz commented Mar 1, 2024 •

edited

Loading

kfaraz Mar 1, 2024

AmatyaAvadhanula Mar 1, 2024

kfaraz Mar 1, 2024

kfaraz left a comment

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz Mar 20, 2024

AmatyaAvadhanula Mar 20, 2024

kfaraz left a comment

AmatyaAvadhanula commented Mar 20, 2024

maytasm commented May 22, 2024

	log.info("Polled and found %,d segments in the database.", segments.size());
	log.info("Polled and found [%,d] segments in the database.", segments.size());

	Stopwatch stopwatch = Stopwatch.createStarted();
	final Stopwatch stopwatch = Stopwatch.createStarted();

Optimize isOvershadowed when there is a unique minor version for an interval #15952

Optimize isOvershadowed when there is a unique minor version for an interval #15952

Conversation

AmatyaAvadhanula commented Feb 23, 2024 • edited Loading

kfaraz left a comment

Choose a reason for hiding this comment

kfaraz commented Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz left a 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

kfaraz left a comment

Choose a reason for hiding this comment

AmatyaAvadhanula commented Mar 20, 2024

maytasm commented May 22, 2024

AmatyaAvadhanula commented Feb 23, 2024 •

edited

Loading

kfaraz commented Mar 1, 2024 •

edited

Loading