[GitHub] [druid] a2l007 commented on a diff in pull request #10877: Allow coordinator to be configured to kill segments in future

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

a2l007 commented on code in PR #10877:
URL: https://github.com/apache/druid/pull/10877#discussion_r863325482


##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java:
##########
@@ -30,15 +30,18 @@
 import org.apache.druid.server.coordinator.DruidCoordinatorConfig;
 import org.apache.druid.server.coordinator.DruidCoordinatorRuntimeParams;
 import org.apache.druid.utils.CollectionUtils;
+import org.joda.time.DateTime;
 import org.joda.time.Interval;
 
 import javax.annotation.Nullable;
 import java.util.Collection;
 import java.util.List;
 
 /**
- * Completely removes information about unused segments whose end time is older than {@link #retainDuration} from now
- * from the metadata store. This action is called "to kill a segment".
+ * Completely removes information about unused segments who have an interval end that comes before
+ * now - {@link #retainDuration}. retainDuration can be a positive or negative duration, negative meaning the interval

Review Comment:
   nit: metadata store part missing after rewrite.



##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java:
##########
@@ -118,17 +129,48 @@ public DruidCoordinatorRuntimeParams run(DruidCoordinatorRuntimeParams params)
     return params;
   }
 
+  /**
+   * For a given datasource and limit of segments that can be killed in one task, determine the interval to be
+   * submitted with the kill task.
+   *
+   * @param dataSource dataSource whose unused segments are being killed.
+   * @param limit the maximum number of segments that can be included in the kill task.
+   * @return {@link Interval} to be used in the kill task.
+   */
   @VisibleForTesting
   @Nullable
   Interval findIntervalForKill(String dataSource, int limit)
   {
     List<Interval> unusedSegmentIntervals =
-        segmentsMetadataManager.getUnusedSegmentIntervals(dataSource, DateTimes.nowUtc().minus(retainDuration), limit);
+        segmentsMetadataManager.getUnusedSegmentIntervals(dataSource, getEndTimeUpperLimit(), limit);
 
     if (unusedSegmentIntervals != null && unusedSegmentIntervals.size() > 0) {
       return JodaUtils.umbrellaInterval(unusedSegmentIntervals);
     } else {
       return null;
     }
   }
+
+  /**
+   * Calculate the {@link DateTime} that wil form the upper bound when looking for segments that are
+   * eligible to be killed. If ignoreDurationToRetain is true, we have no upper bound and return a DateTime object
+   * for 9999-12-31T23:59. This static date has to be used because the metastore is not comparing date objects, but rather
+   * varchar columns. This means DateTimes.MAX is less than the 21st century and beyond for according to the metastore, due to its
+   * year starting with a '1'
+   *
+   * @return {@link DateTime} representing the upper bound time used when looking for segments to kill.
+   */
+  @VisibleForTesting
+  DateTime getEndTimeUpperLimit()
+  {
+    return ignoreRetainDuration
+           ? DateTimes.of(9999, 12, 31, 23, 59)

Review Comment:
   Can we use `DateTimes.CAN_COMPARE_AS_YEAR_MAX` instead? Might be worth renaming that property as well for readability purposes.



-- 
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: commits-unsubscribe@druid.apache.org

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