You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/14 21:57:53 UTC
[GitHub] [druid] jihoonson opened a new pull request #10033: Allow append to existing datasources when dynamic partitioing is used
jihoonson opened a new pull request #10033:
URL: https://github.com/apache/druid/pull/10033
Fixes https://github.com/apache/druid/issues/9352.
### Description
This PR is implemented on top of https://github.com/apache/druid/pull/10025.
This PR allows appending to existing datasources if the appended segments are partitioned using the `dynamic` partitionsSpec. The main change is found in `IndexerSQLMetadataStorageCoordinator`, which used to return null in `createNewSegment()` if the partitionsSpec is not compatible. It now allows segments to have mixed types of shardSpecs and finds the proper partitionId using a new method `ShardSpec.sharePartitionSpace()`.
Most of the changes are new unit tests to verify the behavior. The added tests include
- tuningConfig test: allow only dynamic if appendToExisting = true
- Append test
- With time chunk lock
- Append dynamically partitioned segments to a dynamically partitioned datasource
- Append dynamically partitioned segments to a hash-partitioned datasource
- Append dynamically partitioned segments to a range-partitioned datasource
- Compact segments of mixed shardSpecs
- With segment lock
- Append dynamically partitioned segments to a dynamically partitioned datasource
- Overwrite a datasource with segment lock and append dynamically partitioned segments
- Compact a subset of segments of mixed shardSpecs in each time chunk
Integration tests are still missing, I will add them in a follow-up pr.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [x] 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](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `IndexerSQLMetadataStorageCoordinator`
* `ShardSpec`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10033:
URL: https://github.com/apache/druid/pull/10033#issuecomment-649222738
```
133 F default boolean sharePartitionSpace(PartialShardSpec partialShardSpec)
134 F {
135 F | L | B(0/2) return !partialShardSpec.useNonRootGenerationPartitionSpace();
136 }
```
The test coverage still complains about this default method, but I believe it's being tested in the new tests I added in https://github.com/apache/druid/pull/10033/commits/b423e964d258d2ba2f9760fa3224026157e98d0d.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10033:
URL: https://github.com/apache/druid/pull/10033
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10033:
URL: https://github.com/apache/druid/pull/10033#issuecomment-649804890
> If those unit tests had been in `core` instead of `server` then the coverage check for this PR would have passed since the relevant unit tests were added to `SingleDimensionShardSpecTest`, for example.
>
> I suggest we do a followup PR to move the `ShardSpec` tests from `server` to `core` and proceed with merging this PR, since the coverage check failure is a result of the prior misplacement of test classes.
Ah cool, makes sense. I will do as a follow-up. Thank you for the review @clintropolis @maytasm @ccaominh!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10033:
URL: https://github.com/apache/druid/pull/10033#discussion_r444696192
##########
File path: core/src/test/java/org/apache/druid/timeline/partition/HashBasedNumberedPartialShardSpecTest.java
##########
@@ -73,5 +73,21 @@ public void testJsonPropertyNames() throws IOException
Assert.assertEquals(expected.getPartitionDimensions(), map.get("partitionDimensions"));
Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
Assert.assertEquals(expected.getNumBuckets(), map.get("numPartitions"));
+ Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
+ }
+
+ @Test
+ public void testComplete()
+ {
+ final HashBasedNumberedPartialShardSpec partialShardSpec = new HashBasedNumberedPartialShardSpec(
+ ImmutableList.of("dim"),
+ 2,
+ 4
+ );
+ final ShardSpec shardSpec = partialShardSpec.complete(new ObjectMapper(), 1, 3);
Review comment:
just curious, this doesn't need to change, why does this test use `new ObjectMapper()` instead of `MAPPER`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
ccaominh commented on pull request #10033:
URL: https://github.com/apache/druid/pull/10033#issuecomment-649737980
> ```
> 133 F default boolean sharePartitionSpace(PartialShardSpec partialShardSpec)
> 134 F {
> 135 F | L | B(0/2) return !partialShardSpec.useNonRootGenerationPartitionSpace();
> 136 }
> ```
>
> The test coverage still complains about this default method, but I believe it's being tested in the new tests I added in [b423e96](https://github.com/apache/druid/commit/b423e964d258d2ba2f9760fa3224026157e98d0d).
I think I know what's going on with the code coverage check:
The coverage check is looking for more unit tests in `core`, since that's where `SharedSpec` lives. `NumberedOverwriteShardSpecTest.testSharePartitionSpace()` is in `core`; however, it is using `NumberedOverwriteShardSpec` and since `OverwriteShardSpec` overrides `sharePartitionSpace()`, the test is not covering `ShardSpec.sharePartitionSpace()`. The other unit tests that were added in https://github.com/apache/druid/commit/b423e964d258d2ba2f9760fa3224026157e98d0d are in `server`, so they're not counted.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
ccaominh commented on pull request #10033:
URL: https://github.com/apache/druid/pull/10033#issuecomment-649750336
Looking at the code coverage issue a bit more:
All the ShardSpecs live in `core`:
```
find . -name "*ShardSpec.java"
./core/src/main/java/org/apache/druid/timeline/partition/NumberedOverwriteShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/OverwriteShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/RangeBucketShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/NumberedPartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/HashBucketShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/NoneShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/SingleDimensionShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/NumberedShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/HashBasedNumberedPartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/HashBasedNumberedShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/LinearShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/SingleDimensionPartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/NumberedOverwritePartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/BucketNumberedShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/BuildingHashBasedNumberedShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/BuildingNumberedShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/ShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/PartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/LinearPartialShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/BuildingSingleDimensionShardSpec.java
./core/src/main/java/org/apache/druid/timeline/partition/BuildingShardSpec.java
./server/src/main/java/org/apache/druid/segment/realtime/appenderator/SegmentIdWithShardSpec.java
./indexing-hadoop/src/main/java/org/apache/druid/indexer/HadoopyShardSpec.java
```
But some of their unit tests live in `server` (e.g., `SingleDimensionShardSpecTest`):
```
find . -name "*ShardSpecTest.java"
./core/src/test/java/org/apache/druid/timeline/partition/NumberedOverwritePartialShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/NoneShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/BuildingSingleDimensionShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/BuildingHashBasedNumberedShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/HashBucketShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/SingleDimensionPartialShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/NumberedOverwriteShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/NumberedPartialShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/HashBasedNumberedPartialShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/BuildingNumberedShardSpecTest.java
./core/src/test/java/org/apache/druid/timeline/partition/RangeBucketShardSpecTest.java
./server/src/test/java/org/apache/druid/segment/realtime/appenderator/SegmentIdWithShardSpecTest.java
./server/src/test/java/org/apache/druid/server/shard/NumberedShardSpecTest.java
./server/src/test/java/org/apache/druid/server/shard/SingleDimensionShardSpecTest.java
./server/src/test/java/org/apache/druid/timeline/partition/HashBasedNumberedShardSpecTest.java
```
If those unit tests had been in `core` instead of `server` then the coverage check for this PR would have passed since the relevant unit tests were added to `SingleDimensionShardSpecTest`, for example.
I suggest we do a followup PR to move the `ShardSpec` tests from `server` to `core` and proceed with merging this PR, since the coverage check failure is a result of the prior misplacement of test classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10033:
URL: https://github.com/apache/druid/pull/10033#discussion_r445218905
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/ShardSpec.java
##########
@@ -125,8 +125,13 @@ default short getAtomicUpdateGroupSize()
boolean possibleInDomain(Map<String, RangeSet<String>> domain);
/**
- * Returns true if two segments of this and other shardSpecs can exist in the same time chunk.
+ * Returns true if this shardSpec and the given {@link PartialShardSpec} share the same partition space.
+ * Any implementation of {@link OverwriteShardSpec} should return true.
+ *
+ * @see PartitionIds
*/
- @JsonIgnore
- boolean isCompatible(Class<? extends ShardSpec> other);
+ default boolean sharePartitionSpace(PartialShardSpec partialShardSpec)
+ {
+ return !partialShardSpec.useNonRootGenerationPartitionSpace();
Review comment:
Seems like the java doc for ShardSpec#sharePartitionSpace and OverwriteShardSpec#sharePartitionSpace should be updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10033:
URL: https://github.com/apache/druid/pull/10033#discussion_r445196039
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/ShardSpec.java
##########
@@ -125,8 +125,13 @@ default short getAtomicUpdateGroupSize()
boolean possibleInDomain(Map<String, RangeSet<String>> domain);
/**
- * Returns true if two segments of this and other shardSpecs can exist in the same time chunk.
+ * Returns true if this shardSpec and the given {@link PartialShardSpec} share the same partition space.
+ * Any implementation of {@link OverwriteShardSpec} should return true.
+ *
+ * @see PartitionIds
*/
- @JsonIgnore
- boolean isCompatible(Class<? extends ShardSpec> other);
+ default boolean sharePartitionSpace(PartialShardSpec partialShardSpec)
+ {
+ return !partialShardSpec.useNonRootGenerationPartitionSpace();
Review comment:
Should this be `partialShardSpec.useNonRootGenerationPartitionSpace();` instead of `!partialShardSpec.useNonRootGenerationPartitionSpace();` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10033:
URL: https://github.com/apache/druid/pull/10033#discussion_r444696192
##########
File path: core/src/test/java/org/apache/druid/timeline/partition/HashBasedNumberedPartialShardSpecTest.java
##########
@@ -73,5 +73,21 @@ public void testJsonPropertyNames() throws IOException
Assert.assertEquals(expected.getPartitionDimensions(), map.get("partitionDimensions"));
Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
Assert.assertEquals(expected.getNumBuckets(), map.get("numPartitions"));
+ Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
+ }
+
+ @Test
+ public void testComplete()
+ {
+ final HashBasedNumberedPartialShardSpec partialShardSpec = new HashBasedNumberedPartialShardSpec(
+ ImmutableList.of("dim"),
+ 2,
+ 4
+ );
+ final ShardSpec shardSpec = partialShardSpec.complete(new ObjectMapper(), 1, 3);
Review comment:
why do this test use `new ObjectMapper()` instead of `MAPPER`?
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/RangePartitionMultiPhaseParallelIndexingTest.java
##########
@@ -201,48 +208,122 @@ private static void writeRow(
public void createsCorrectRangePartitions() throws Exception
{
int targetRowsPerSegment = NUM_ROW / DIM_FILE_CARDINALITY / NUM_PARTITION;
- final Set<DataSegment> publishedSegments;
+ final Set<DataSegment> publishedSegments = runTestTask(
+ new SingleDimensionPartitionsSpec(
+ targetRowsPerSegment,
+ null,
+ DIM1,
+ false
+ ),
+ useMultivalueDim ? TaskState.FAILED : TaskState.SUCCESS,
+ false
+ );
+
+ if (!useMultivalueDim) {
+ assertRangePartitions(publishedSegments);
+ }
+ }
+
+ @Test
+ public void testAppendLinearlyPartitionedSegmensToHashPartitionedDatasourceSuccessfullyAppend()
Review comment:
nit, typo: `Segmens`
"testAppendLinearlyPartitionedSegmentsToHashPartitionedDatasourceSuccessfullyAppend"
##########
File path: server/src/test/java/org/apache/druid/server/shard/SingleDimensionShardSpecTest.java
##########
@@ -142,6 +146,16 @@ public void testPossibleInDomain()
Assert.assertTrue(shard7.possibleInDomain(domain2));
}
+ @Test
+ public void testSharePartitionSpace()
+ {
+ final SingleDimensionShardSpec shardSpec = makeSpec("start", "end");
+ Assert.assertTrue(shardSpec.sharePartitionSpace(NumberedPartialShardSpec.instance()));
+ Assert.assertTrue(shardSpec.sharePartitionSpace(new HashBasedNumberedPartialShardSpec(null, 0, 1)));
+ Assert.assertTrue(shardSpec.sharePartitionSpace(new SingleDimensionPartialShardSpec("dim", 0, null, null, 1)));
Review comment:
How do we end up with these mixed shard spec situations, minor compaction? Like I understand why these evaluate to true given how the checks work, just trying to understand how it happens since afaict only dynamic can be appended.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10033: Allow append to existing datasources when dynamic partitioning is used
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10033:
URL: https://github.com/apache/druid/pull/10033#discussion_r445310128
##########
File path: core/src/test/java/org/apache/druid/timeline/partition/HashBasedNumberedPartialShardSpecTest.java
##########
@@ -73,5 +73,21 @@ public void testJsonPropertyNames() throws IOException
Assert.assertEquals(expected.getPartitionDimensions(), map.get("partitionDimensions"));
Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
Assert.assertEquals(expected.getNumBuckets(), map.get("numPartitions"));
+ Assert.assertEquals(expected.getBucketId(), map.get("bucketId"));
+ }
+
+ @Test
+ public void testComplete()
+ {
+ final HashBasedNumberedPartialShardSpec partialShardSpec = new HashBasedNumberedPartialShardSpec(
+ ImmutableList.of("dim"),
+ 2,
+ 4
+ );
+ final ShardSpec shardSpec = partialShardSpec.complete(new ObjectMapper(), 1, 3);
Review comment:
Ah, I didn't notice it while copying these tests 😅
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/RangePartitionMultiPhaseParallelIndexingTest.java
##########
@@ -201,48 +208,122 @@ private static void writeRow(
public void createsCorrectRangePartitions() throws Exception
{
int targetRowsPerSegment = NUM_ROW / DIM_FILE_CARDINALITY / NUM_PARTITION;
- final Set<DataSegment> publishedSegments;
+ final Set<DataSegment> publishedSegments = runTestTask(
+ new SingleDimensionPartitionsSpec(
+ targetRowsPerSegment,
+ null,
+ DIM1,
+ false
+ ),
+ useMultivalueDim ? TaskState.FAILED : TaskState.SUCCESS,
+ false
+ );
+
+ if (!useMultivalueDim) {
+ assertRangePartitions(publishedSegments);
+ }
+ }
+
+ @Test
+ public void testAppendLinearlyPartitionedSegmensToHashPartitionedDatasourceSuccessfullyAppend()
Review comment:
Thanks 👍
##########
File path: server/src/test/java/org/apache/druid/server/shard/SingleDimensionShardSpecTest.java
##########
@@ -142,6 +146,16 @@ public void testPossibleInDomain()
Assert.assertTrue(shard7.possibleInDomain(domain2));
}
+ @Test
+ public void testSharePartitionSpace()
+ {
+ final SingleDimensionShardSpec shardSpec = makeSpec("start", "end");
+ Assert.assertTrue(shardSpec.sharePartitionSpace(NumberedPartialShardSpec.instance()));
+ Assert.assertTrue(shardSpec.sharePartitionSpace(new HashBasedNumberedPartialShardSpec(null, 0, 1)));
+ Assert.assertTrue(shardSpec.sharePartitionSpace(new SingleDimensionPartialShardSpec("dim", 0, null, null, 1)));
Review comment:
Your understanding is correct. I added `HashBasedNumberedPartialShardSpec` and `SingleDimensionPartialShardSpec` in #7547 but they are not in use yet. They will be used once we support append with non-dynamic or non-dynamic with segment locking.
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/ShardSpec.java
##########
@@ -125,8 +125,13 @@ default short getAtomicUpdateGroupSize()
boolean possibleInDomain(Map<String, RangeSet<String>> domain);
/**
- * Returns true if two segments of this and other shardSpecs can exist in the same time chunk.
+ * Returns true if this shardSpec and the given {@link PartialShardSpec} share the same partition space.
+ * Any implementation of {@link OverwriteShardSpec} should return true.
+ *
+ * @see PartitionIds
*/
- @JsonIgnore
- boolean isCompatible(Class<? extends ShardSpec> other);
+ default boolean sharePartitionSpace(PartialShardSpec partialShardSpec)
+ {
+ return !partialShardSpec.useNonRootGenerationPartitionSpace();
Review comment:
Ah, all `ShardSpec`s share the same root-generation partition space except `OverwriteShardSpec`. So, the current code is correct. I updated the javadoc to make it more clear.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org