Allow large partition numbers in VersionedIntervalTimeline. #18777

gianm · 2025-11-24T06:59:44Z

The experimental segment locking feature (forceTimeChunkLock: false) reserves the first 32768 partition numbers for the "root generation", and then uses an "atomic update groups" scheme to replace root segment ranges with new sets of segments during reindexing operations.

OvershadowableManager, which manages this atomic update scheme, imposes a limit of 32768 segments per time chunk. This applies even to people that are not using segment locking. In this patch, the class is now only used when segment locking is actually in play, meaning that the limit is not imposed under normal conditions.

The experimental segment locking feature (forceTimeChunkLock: false) reserves the first 32768 partition numbers for the "root generation", and then uses an "atomic update groups" scheme to replace root segment ranges with new sets of segments during reindexing operations. OvershadowableManager, which manages this atomic update scheme, imposes a limit of 32768 segments per time chunk. Previously, this applied even to people that are not using segment locking. In this patch, the class is now only used when segment locking is actually in play, meaning that the limit is not imposed under normal conditions.

kfaraz

Looks neat! It is nice to have a clear distinction between when the minor versions are in use and when they are not.

Left a couple of suggestions. Let me know what you think.

kfaraz · 2025-11-24T07:18:20Z

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

-    boolean added = overshadowableManager.addChunk(chunk);
+    if (chunk.getObject().getMinorVersion() != 0 && contents instanceof SimplePartitionHolderContents) {
+      // Swap simple map for an OvershadowableManager when minor versions are encountered.
+      contents = OvershadowableManager.fromSimple((SimplePartitionHolderContents<T>) contents);


Since this swap will bring about changes in the segment-holding capacity of the interval, should we add a log line here?

I think it'll get logged too much, since timelines are created all the time.

No, I meant log something only when we change from simple to minor-version enabled. Would that be verbose too?

I think so, since timelines are created and populated as part of various operations. It would get logged semi-randomly and perhaps too frequently.

kfaraz · 2025-11-24T07:21:51Z

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

 * This class is not thread-safe.
 */
-class OvershadowableManager<T extends Overshadowable<T>>
+class OvershadowableManager<T extends Overshadowable<T>> implements PartitionHolderContents<T>


Maybe rename this to indicate that it is used only when segment locking is used.

Hmm, rather than rename I added a javadoc.

kfaraz · 2025-11-24T10:37:43Z

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

+   * {@link PartitionHolder#isComplete()}. When segment locking is not being used, all chunks
+   * are consistent, so this always returns true.
+   */
+  boolean visibleChunksAreConsistent();


Nit:

Suggested change

boolean visibleChunksAreConsistent();

boolean areVisibleChunksConsistent();

kfaraz · 2025-11-24T10:42:28Z

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

+ * @see SimplePartitionHolderContents implementation when segment locking is not in play
+ * @see OvershadowableManager implementation when segment locking is in play
+ */
+public interface PartitionHolderContents<T>


Super Nit: "Holder" + "Contents" sound like they should cancel each other out. 😛

Should we call this class PartitionChunks<T> or PartitionChunkGroup<T> since it is basically just a group of chunks?

The implementations could be called SimplePartitionChunkGroup and PartitionChunkGroupWithMinorVersion or something.

"Group" unfortunately means something different in OvershadowableManager (an atomic update group).

kfaraz · 2025-11-24T10:44:05Z

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

+  private final TreeMap<PartitionChunk<T>, PartitionChunk<T>> holderMap = new TreeMap<>();
+
+  /**
+   * Map of {@link PartitionChunk#getChunkNumber()} to


Suggested change

* Map of {@link PartitionChunk#getChunkNumber()} to

* Map from {@link PartitionChunk#getChunkNumber()} to the actual {@link PartitionChunk}.

kfaraz · 2025-11-24T10:50:06Z

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

  }

-  public static <T extends Overshadowable<T>> OvershadowableManager<T> copyVisible(OvershadowableManager<T> original)
+  public static <T extends Overshadowable<T>> OvershadowableManager<T> fromSimple(


Should this method fail if the passed simple holder already has more than 32k chunks?

I just added a check to addChunk, so now it will.

embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/HighPartitionNumberTest.java

+    final int maxPartitionNum = Integer.parseInt(cluster.runSql(
+        "SELECT MAX(partition_num) FROM sys.segments WHERE datasource=%s",
+        Calcites.escapeStringLiteral(dataSource)
+    ).trim());


kfaraz · 2025-11-25T12:11:21Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/config/TaskLockConfig.java

+   * individually create each segment. Controls the initial partition number to be used for segment allocation.
+   */
+  @JsonProperty
+  private int initialAllocationPartitionNumber = PartitionIds.ROOT_GEN_START_PARTITION_ID;


If this config is to be used only for tests, I would advise a slightly different strategy which wouldn't require touching all the production classes.

Use the ClusterTestingModule extension in the embedded test and enable it on the Overlord via druid.unsafe.cluster.testing=true.

Create a new class FaultyIndexerSQLMetadataStorageCoordinator extends IndexerSQLMetadataStorageCoordinator and override allocation methods.

In ClusterTestingModule, do binder.bind(IndexerSQLMetadataStorageCoordinator.class).to(FaultyIndexer...);

Pass a test config into the faulty implementation to control the startPartitionId.

I just pushed a different approach:

First insert the test data into a single segment using SQL, with partition 0.

Direct insert a new segment into metadata storage that is the same as that first one, but with a partition number of Short.MAX_VALUE

Run SQL to insert 3 more segments.

Verify all are loaded and queryable.

I removed this new parameter I had added.

gianm · 2025-11-25T17:24:52Z

@kfaraz just updated this— TY for review.

kfaraz · 2025-11-26T04:29:20Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/HighPartitionNumberTest.java

+        Set.of(
+            firstSegment.withShardSpec(
+                new NumberedShardSpec(
+                    Short.MAX_VALUE - 1,


Nice trick!

kfaraz · 2025-11-26T04:29:56Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/HighPartitionNumberTest.java

+    insertSecondSegment();
+    insertLastSegments();
+
+


Nit: extra newline?

gianm added 2 commits November 23, 2025 22:59

Use the correct annotation.

e041e17

kfaraz reviewed Nov 24, 2025

View reviewed changes

Add embedded test, and config property to support it.

42eb197

github-actions bot added the Area - Ingestion label Nov 25, 2025

github-advanced-security bot found potential problems Nov 25, 2025

View reviewed changes

kfaraz reviewed Nov 25, 2025

View reviewed changes

gianm added 2 commits November 25, 2025 09:17

Do HighPartitionNumberTest with a different approach.

b031d44

Changes from review.

9d93d34

Align behavior with tests.

2818177

kfaraz approved these changes Nov 26, 2025

View reviewed changes

gianm added 2 commits November 25, 2025 20:40

Remove extraneous newline.

aaa8efd

Fix comment.

d226805

capistrant merged commit f3ecfc7 into apache:master Nov 26, 2025
57 checks passed

gianm deleted the segment-locking-off2 branch November 29, 2025 05:22

kgyrtkirk added this to the 36.0.0 milestone Jan 19, 2026

	boolean visibleChunksAreConsistent();
	boolean areVisibleChunksConsistent();

	* Map of {@link PartitionChunk#getChunkNumber()} to
	* Map from {@link PartitionChunk#getChunkNumber()} to the actual {@link PartitionChunk}.

Allow large partition numbers in VersionedIntervalTimeline. #18777

Allow large partition numbers in VersionedIntervalTimeline. #18777

Uh oh!

Conversation

gianm commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Uh oh!

Check notice

kfaraz Nov 25, 2025 • 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!

gianm commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gianm commented Nov 24, 2025 •

edited

Loading

kfaraz Nov 25, 2025 •

edited

Loading