[GitHub] [druid] jihoonson commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity

[GitBox <gi...@apache.org>]

jihoonson commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597276407



##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,34 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
       needsCompaction = true;
     }
 
-    // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
-    final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
-                                           objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
-                                           null;
-
-    if (config.getGranularitySpec() != null &&
-        config.getGranularitySpec().getSegmentGranularity() != null &&
-        !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
-      log.info(
-          "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
-          config.getGranularitySpec(),
-          segmentGranularity
-      );
-      needsCompaction = true;
+    if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+      // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+      final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+                                                     objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+                                                     null;
+      if (existingSegmentGranularity == null) {
+        // Candidate segments were all compacted without segment granularity set.
+        // We need to check if all segments have the same segment granularity and if it is the same
+        // as the configured segment granularity.
+        Set<Granularity> segmentGranularities = candidates.segments.stream()
+                                                                   .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())

Review comment:
       `GranularityType.fromPeriod()` can throw an exception if the segment interval is not one of the enums in `GranularityType`. I think we should probably compare the duration of the granularity as well as their timezone and origin.




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