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: [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]
