[GitHub] [druid] kfaraz commented on a diff in pull request #13391: Make reservoir segment sampling the default, minor cleanup of coordinator config

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

kfaraz commented on code in PR #13391:
URL: https://github.com/apache/druid/pull/13391#discussion_r1027334166


##########
server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java:
##########
@@ -20,281 +20,206 @@
 package org.apache.druid.server.coordinator.duty;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import org.apache.druid.client.indexing.IndexingServiceClient;
 import org.apache.druid.java.util.common.DateTimes;
-import org.apache.druid.java.util.common.Intervals;
 import org.apache.druid.metadata.SegmentsMetadataManager;
 import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
 import org.apache.druid.server.coordinator.DruidCoordinatorConfig;
 import org.apache.druid.server.coordinator.DruidCoordinatorRuntimeParams;
-import org.apache.druid.server.coordinator.TestDruidCoordinatorConfig;
-import org.easymock.EasyMock;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.partition.NoneShardSpec;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.joda.time.Interval;
-import org.junit.Assert;
+import org.joda.time.Period;
 import org.junit.Before;
 import org.junit.Test;
-import org.junit.experimental.runners.Enclosed;
 import org.junit.runner.RunWith;
 import org.mockito.Answers;
+import org.mockito.ArgumentMatchers;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
-import java.util.Set;
+import java.util.stream.Collectors;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 
 /**
+ *
  */
-@RunWith(Enclosed.class)
+@RunWith(MockitoJUnitRunner.class)
 public class KillUnusedSegmentsTest
 {
-  /**
-   * Standing up new tests with mocks was easier than trying to move the existing tests to use mocks for consistency.
-   * In the future, if all tests are moved to use the same structure, this inner static class can be gotten rid of.
-   */
-  @RunWith(MockitoJUnitRunner.class)
-  public static class MockedTest
+  private static final int MAX_SEGMENTS_TO_KILL = 10;
+  private static final Duration COORDINATOR_KILL_PERIOD = Duration.standardMinutes(2);
+  private static final Duration DURATION_TO_RETAIN = Duration.standardDays(1);
+  private static final Duration INDEXING_PERIOD = Duration.standardMinutes(1);
+
+  @Mock
+  private SegmentsMetadataManager segmentsMetadataManager;
+  @Mock
+  private IndexingServiceClient indexingServiceClient;
+  @Mock(answer = Answers.RETURNS_DEEP_STUBS)
+  private DruidCoordinatorConfig config;
+
+  @Mock
+  private DruidCoordinatorRuntimeParams params;
+  @Mock
+  private CoordinatorDynamicConfig coordinatorDynamicConfig;
+
+  private DataSegment yearOldSegment;
+  private DataSegment monthOldSegment;
+  private DataSegment dayOldSegment;
+  private DataSegment hourOldSegment;
+  private DataSegment nextDaySegment;
+  private DataSegment nextMonthSegment;
+
+  private KillUnusedSegments target;
+
+  @Before
+  public void setup()
   {
-    private static final Set<String> ALL_DATASOURCES = ImmutableSet.of("DS1", "DS2", "DS3");
-    private static final int MAX_SEGMENTS_TO_KILL = 10;
-    private static final Duration COORDINATOR_KILL_PERIOD = Duration.standardMinutes(2);
-    private static final Duration DURATION_TO_RETAIN = Duration.standardDays(1);
-    private static final Duration INDEXING_PERIOD = Duration.standardMinutes(1);
-
-    @Mock
-    private SegmentsMetadataManager segmentsMetadataManager;
-    @Mock
-    private IndexingServiceClient indexingServiceClient;
-    @Mock(answer = Answers.RETURNS_DEEP_STUBS)
-    private DruidCoordinatorConfig config;
-
-    @Mock
-    private DruidCoordinatorRuntimeParams params;
-    @Mock
-    private CoordinatorDynamicConfig coordinatorDynamicConfig;
-    private KillUnusedSegments target;
-
-    @Before
-    public void setup()
-    {
-      Mockito.doReturn(coordinatorDynamicConfig).when(params).getCoordinatorDynamicConfig();
-      Mockito.doReturn(ALL_DATASOURCES).when(segmentsMetadataManager).retrieveAllDataSourceNames();
-      Mockito.doReturn(COORDINATOR_KILL_PERIOD).when(config).getCoordinatorKillPeriod();
-      Mockito.doReturn(DURATION_TO_RETAIN).when(config).getCoordinatorKillDurationToRetain();
-      Mockito.doReturn(INDEXING_PERIOD).when(config).getCoordinatorIndexingPeriod();
-      Mockito.doReturn(MAX_SEGMENTS_TO_KILL).when(config).getCoordinatorKillMaxSegments();
-      target = new KillUnusedSegments(segmentsMetadataManager, indexingServiceClient, config);
-    }
-    @Test
-    public void testRunWihNoIntervalShouldNotKillAnySegments()
-    {
-      target.run(params);
-      Mockito.verify(indexingServiceClient, Mockito.never())
-             .killUnusedSegments(anyString(), anyString(), any(Interval.class));
-    }
-
-    @Test
-    public void testRunWihSpecificDatasourceAndNoIntervalShouldNotKillAnySegments()
-    {
-      Mockito.when(coordinatorDynamicConfig.getSpecificDataSourcesToKillUnusedSegmentsIn()).thenReturn(Collections.singleton("DS1"));
-      target.run(params);
-      Mockito.verify(indexingServiceClient, Mockito.never())
-             .killUnusedSegments(anyString(), anyString(), any(Interval.class));
-    }
+    Mockito.doReturn(coordinatorDynamicConfig).when(params).getCoordinatorDynamicConfig();
+    Mockito.doReturn(COORDINATOR_KILL_PERIOD).when(config).getCoordinatorKillPeriod();
+    Mockito.doReturn(DURATION_TO_RETAIN).when(config).getCoordinatorKillDurationToRetain();
+    Mockito.doReturn(INDEXING_PERIOD).when(config).getCoordinatorIndexingPeriod();
+    Mockito.doReturn(MAX_SEGMENTS_TO_KILL).when(config).getCoordinatorKillMaxSegments();
+
+    Mockito.doReturn(Collections.singleton("DS1"))
+           .when(coordinatorDynamicConfig).getSpecificDataSourcesToKillUnusedSegmentsIn();
+
+    final DateTime now = DateTimes.nowUtc();
+
+    yearOldSegment = createSegmentWithEnd(now.minusDays(365));
+    monthOldSegment = createSegmentWithEnd(now.minusDays(30));
+    dayOldSegment = createSegmentWithEnd(now.minusDays(1));
+    hourOldSegment = createSegmentWithEnd(now.minusHours(1));
+    nextDaySegment = createSegmentWithEnd(now.plusDays(1));
+    nextMonthSegment = createSegmentWithEnd(now.plusDays(30));
+
+    final List<DataSegment> unusedSegments = ImmutableList.of(
+        yearOldSegment,
+        monthOldSegment,
+        dayOldSegment,
+        hourOldSegment,
+        nextDaySegment,
+        nextMonthSegment
+    );
+
+    Mockito.when(
+        segmentsMetadataManager.getUnusedSegmentIntervals(
+            ArgumentMatchers.anyString(),
+            ArgumentMatchers.any(),
+            ArgumentMatchers.anyInt()
+        )
+    ).thenAnswer(invocation -> {
+      DateTime maxEndTime = invocation.getArgument(1);
+      return unusedSegments.stream()
+                           .map(DataSegment::getInterval)
+                           .filter(i -> i.getEnd().isBefore(maxEndTime))
+                           .collect(Collectors.toList());
+    });
+
+    target = new KillUnusedSegments(segmentsMetadataManager, indexingServiceClient, config);
   }
 
-  public static class FindIntervalsTest
+  @Test
+  public void testRunWithNoIntervalShouldNotKillAnySegments()
   {
-    @Test
-    public void testFindIntervalForKill()

Review Comment:
   This test essentially just verifies the `JodaUtils.umbrellaInterval()` method and these cases are already being verified in `JodaUtilsTest`.
   The required test cases from here have been merged into the other tests in this class.



##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -115,7 +116,7 @@ public CoordinatorDynamicConfig(
       @JsonProperty("mergeSegmentsLimit") int mergeSegmentsLimit,
       @JsonProperty("maxSegmentsToMove") int maxSegmentsToMove,
       @Deprecated @JsonProperty("percentOfSegmentsToConsiderPerMove") @Nullable Double percentOfSegmentsToConsiderPerMove,
-      @JsonProperty("useBatchedSegmentSampler") boolean useBatchedSegmentSampler,
+      @Deprecated @JsonProperty("useBatchedSegmentSampler") boolean useBatchedSegmentSampler,

Review Comment:
   Thanks for catching this! It was in a different patch. 
   Updated to use the default of `true` both when
   - deserializing using the constructor. This would mean that configs already stored in the DB with no explicit value of `useBatchedSegmentSampler` would now start using `true`.
   - creating/deserializing using the Builder. So newly created/updated configs would now  use `true`.
   
   Also updated tests to verify this.
   



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