suneet-s commented on a change in pull request #11874:
URL: https://github.com/apache/druid/pull/11874#discussion_r743839200
##########
File path: docs/configuration/index.md
##########
@@ -988,7 +989,7 @@ The below is a list of the supported configurations for
auto compaction.
|`chatHandlerNumRetries`|Retries for reporting the pushed segments in worker
tasks.|no (default = 5)|
###### Automatic compaction granularitySpec
-You can optionally use the `granularitySpec` object to configure the segment
granularity of the compacted segments.
+You can optionally use the `granularitySpec` object to configure the
granularity and rollup of the compacted segments.
Review comment:
nit: These fields are defined below, so it seems redundant to mention
them again here.
##########
File path: core/src/test/java/org/apache/druid/timeline/DataSegmentTest.java
##########
@@ -120,6 +121,7 @@ public void testV1Serialization() throws Exception
new NumberedShardSpec(3, 0),
new CompactionState(
new HashedPartitionsSpec(100000, null, ImmutableList.of("dim1")),
+ new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(ImmutableList.of("dim1", "bar",
"foo")), null, null),
Review comment:
Can we add a test for de-serialization of a CompactionState object that
does not have the `dimensionsSpec` field
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -440,6 +442,19 @@ private boolean needsCompaction(DataSourceCompactionConfig
config, SegmentsToCom
}
}
+ if (config.getDimensionsSpec() != null) {
+ final DimensionsSpec existingDimensionsSpec =
lastCompactionState.getDimensionsSpec();
+ // Checks for list of dimensions
+ if (config.getDimensionsSpec().getDimensions() != null) {
+ final List<DimensionSchema> existingDimensions =
existingDimensionsSpec != null ?
+
existingDimensionsSpec.getDimensions() :
+ null;
+ if
(!config.getDimensionsSpec().getDimensions().equals(existingDimensions)) {
+ return true;
Review comment:
nit: Please add a log line here for operators to know why the segment
was selected for compaction similar to segmentGranularity above.
Similar comments for rollup + queryGranularity
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -440,6 +442,19 @@ private boolean needsCompaction(DataSourceCompactionConfig
config, SegmentsToCom
}
}
+ if (config.getDimensionsSpec() != null) {
+ final DimensionsSpec existingDimensionsSpec =
lastCompactionState.getDimensionsSpec();
+ // Checks for list of dimensions
+ if (config.getDimensionsSpec().getDimensions() != null) {
Review comment:
If `config.getDimensionsSpec().getDimensions()` == null and
lastCompactionState.getDimensionsSpec() is non-null then we want
needsCompaction to return false. Is that understanding correct?
--
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]
