kfaraz commented on code in PR #18777:
URL: https://github.com/apache/druid/pull/18777#discussion_r2554882540
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java:
##########
@@ -41,42 +45,44 @@ public static <T extends Overshadowable<T>>
PartitionHolder<T> copyWithOnlyVisib
)
{
return new PartitionHolder<>(
-
OvershadowableManager.copyVisible(partitionHolder.overshadowableManager),
+ partitionHolder.contents.copyVisible(),
partitionHolder.maxMinorVersion
);
}
public static <T extends Overshadowable<T>> PartitionHolder<T>
deepCopy(PartitionHolder<T> partitionHolder)
{
return new PartitionHolder<>(
- OvershadowableManager.deepCopy(partitionHolder.overshadowableManager),
+ partitionHolder.contents.deepCopy(),
partitionHolder.maxMinorVersion
);
}
public PartitionHolder(PartitionChunk<T> initialChunk)
{
- this.overshadowableManager = new OvershadowableManager<>();
add(initialChunk);
}
public PartitionHolder(List<PartitionChunk<T>> initialChunks)
{
- this.overshadowableManager = new OvershadowableManager<>();
for (PartitionChunk<T> chunk : initialChunks) {
add(chunk);
}
}
- protected PartitionHolder(OvershadowableManager<T> overshadowableManager,
short maxMinorVersion)
+ protected PartitionHolder(PartitionHolderContents<T> contents, short
maxMinorVersion)
{
- this.overshadowableManager = overshadowableManager;
+ this.contents = contents;
this.maxMinorVersion = maxMinorVersion;
}
public boolean add(PartitionChunk<T> chunk)
{
- 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);
Review Comment:
Since this swap will bring about changes in the segment-holding capacity of
the interval, should we add a log line here?
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolderContents.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.timeline.partition;
+
+import javax.annotation.Nullable;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * Contents for {@link PartitionHolder}.
+ *
+ * @see SimplePartitionHolderContents implementation when segment locking is
not in play
+ * @see OvershadowableManager implementation when segment locking is in play
+ */
+public interface PartitionHolderContents<T>
+{
+ /**
+ * Whether this holder is empty.
+ */
+ boolean isEmpty();
+
+ /**
+ * Whether all visible chunks are consistent, meaning they can possibly be
considered for
+ * {@link PartitionHolder#isComplete()}. When segment locking is not being
used, all chunks
+ * are consistent, so this always returns true.
+ */
+ boolean visibleChunksAreConsistent();
Review Comment:
Nit:
```suggestion
boolean areVisibleChunksConsistent();
```
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -65,7 +65,7 @@
*
* This class is not thread-safe.
*/
-class OvershadowableManager<T extends Overshadowable<T>>
+class OvershadowableManager<T extends Overshadowable<T>> implements
PartitionHolderContents<T>
Review Comment:
Maybe rename this to indicate that it is used only when segment locking is
used.
##########
processing/src/main/java/org/apache/druid/timeline/partition/SimplePartitionHolderContents.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.timeline.partition;
+
+import it.unimi.dsi.fastutil.ints.Int2ObjectMap;
+import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.timeline.Overshadowable;
+
+import javax.annotation.Nullable;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Objects;
+import java.util.TreeMap;
+
+/**
+ * Contents for {@link PartitionHolder} when segment locking was not used, and
therefore no chunks have
+ * {@link Overshadowable#getMinorVersion()}.
+ */
+public class SimplePartitionHolderContents<T extends Overshadowable<T>>
implements PartitionHolderContents<T>
+{
+ private final TreeMap<PartitionChunk<T>, PartitionChunk<T>> holderMap = new
TreeMap<>();
+
+ /**
+ * Map of {@link PartitionChunk#getChunkNumber()} to
Review Comment:
```suggestion
* Map from {@link PartitionChunk#getChunkNumber()} to the actual {@link
PartitionChunk}.
```
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolderContents.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.timeline.partition;
+
+import javax.annotation.Nullable;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * Contents for {@link PartitionHolder}.
+ *
+ * @see SimplePartitionHolderContents implementation when segment locking is
not in play
+ * @see OvershadowableManager implementation when segment locking is in play
+ */
+public interface PartitionHolderContents<T>
Review Comment:
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.
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -105,10 +105,23 @@ enum State
this.overshadowedGroups = new TreeMap<>();
}
- public static <T extends Overshadowable<T>> OvershadowableManager<T>
copyVisible(OvershadowableManager<T> original)
+ public static <T extends Overshadowable<T>> OvershadowableManager<T>
fromSimple(
Review Comment:
Should this method fail if the passed simple holder already has more than
32k chunks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
