From 2389128c5deeac438ee3e5e6a49a5e8e463bf2a7 Mon Sep 17 00:00:00 2001 From: Lucas Capistrant Date: Fri, 14 Aug 2020 15:24:10 -0500 Subject: [PATCH 1/8] allow the LogUsedSegments duty to be skippped --- docs/configuration/index.md | 1 + .../server/coordinator/DruidCoordinator.java | 16 +++++++++++++--- .../coordinator/DruidCoordinatorConfig.java | 4 ++++ .../coordinator/CuratorDruidCoordinatorTest.java | 3 ++- .../coordinator/DruidCoordinatorConfigTest.java | 3 +++ .../server/coordinator/DruidCoordinatorTest.java | 3 ++- .../coordinator/HttpLoadQueuePeonTest.java | 3 ++- .../server/coordinator/LoadQueuePeonTest.java | 6 ++++-- .../server/coordinator/LoadQueuePeonTester.java | 3 ++- .../coordinator/TestDruidCoordinatorConfig.java | 11 ++++++++++- .../coordinator/duty/KillUnusedSegmentsTest.java | 3 ++- 11 files changed, 45 insertions(+), 11 deletions(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index b02c467141a1..0e5375f6482d 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -689,6 +689,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| +|`druid.coordinator.duties.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the LogUsedSegments Duty|true| ##### Segment Management |Property|Possible Values|Description|Default| diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java index 806ba043204f..552d78e4f05b 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java @@ -617,21 +617,31 @@ private void stopBeingLeader() private List makeHistoricalManagementDuties() { - return ImmutableList.of( - new LogUsedSegments(), + List duties = new ArrayList<>(); + if (config.isLogUsedSegmentsDutyEnabled()) { + duties.add(new LogUsedSegments()); + } + duties.addAll(ImmutableList.of( new UpdateCoordinatorStateAndPrepareCluster(), new RunRules(DruidCoordinator.this), new UnloadUnusedSegments(), new MarkAsUnusedOvershadowedSegments(DruidCoordinator.this), new BalanceSegments(DruidCoordinator.this), new EmitClusterStatsAndMetrics(DruidCoordinator.this) + )); + log.debug( + "Done making historical management duties %s", + duties.stream().map(duty -> duty.getClass().getName()).collect(Collectors.toList()) ); + return duties; } private List makeIndexingServiceDuties() { List duties = new ArrayList<>(); - duties.add(new LogUsedSegments()); + if (config.isLogUsedSegmentsDutyEnabled()) { + duties.add(new LogUsedSegments()); + } duties.addAll(makeCompactSegmentsDuty()); duties.addAll(indexingServiceDuties); diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java index 130d7e88d840..3b24da2d7fc8 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java @@ -51,6 +51,10 @@ public abstract class DruidCoordinatorConfig @Default("0") public abstract int getCoordinatorKillMaxSegments(); + @Config("druid.coordinator.duties.logUsedSegments.enabled") + @Default("true") + public abstract boolean isLogUsedSegmentsDutyEnabled(); + @Config("druid.coordinator.load.timeout") public Duration getLoadTimeoutDelay() { diff --git a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java index c44c0d8d3fb3..28bae21a6518 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java @@ -171,7 +171,8 @@ public void setUp() throws Exception new Duration(COORDINATOR_PERIOD), null, 10, - new Duration("PT0s") + new Duration("PT0s"), + false ); sourceLoadQueueChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java index ad563410fe61..9b8e4910fe42 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java @@ -47,6 +47,7 @@ public void testDeserialization() Assert.assertEquals(0, config.getCoordinatorKillMaxSegments()); Assert.assertEquals(new Duration(15 * 60 * 1000), config.getLoadTimeoutDelay()); Assert.assertEquals(Duration.millis(50), config.getLoadQueuePeonRepeatDelay()); + Assert.assertTrue(config.isLogUsedSegmentsDutyEnabled()); //with non-defaults Properties props = new Properties(); @@ -60,6 +61,7 @@ public void testDeserialization() props.setProperty("druid.coordinator.kill.pendingSegments.on", "true"); props.setProperty("druid.coordinator.load.timeout", "PT1s"); props.setProperty("druid.coordinator.loadqueuepeon.repeatDelay", "PT0.100s"); + props.setProperty("druid.coordinator.duties.logUsedSegments.enabled", "false"); factory = Config.createFactory(props); config = factory.build(DruidCoordinatorConfig.class); @@ -72,5 +74,6 @@ public void testDeserialization() Assert.assertEquals(10000, config.getCoordinatorKillMaxSegments()); Assert.assertEquals(new Duration("PT1s"), config.getLoadTimeoutDelay()); Assert.assertEquals(Duration.millis(100), config.getLoadQueuePeonRepeatDelay()); + Assert.assertFalse(config.isLogUsedSegmentsDutyEnabled()); } } diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java index 51308cbd8fa9..7886cd851fae 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java @@ -142,7 +142,8 @@ public void setUp() throws Exception new Duration(COORDINATOR_PERIOD), null, 10, - new Duration("PT0s") + new Duration("PT0s"), + false ); pathChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java index 10d7ba24c40e..97e72eda3273 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java @@ -81,7 +81,8 @@ public class HttpLoadQueuePeonTest null, null, 10, - Duration.ZERO + Duration.ZERO, + false ) { @Override diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java index 2afad8d4dea7..7e30c0d1f8f7 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java @@ -96,7 +96,8 @@ public void testMultipleLoadDropSegments() throws Exception null, null, 10, - Duration.millis(0) + Duration.millis(0), + false ) ); @@ -290,7 +291,8 @@ public void testFailAssign() throws Exception null, null, 10, - new Duration("PT1s") + new Duration("PT1s"), + false ) ); diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java index fb9287320dca..e101af8ee28b 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java @@ -45,7 +45,8 @@ public LoadQueuePeonTester() null, null, 10, - new Duration("PT1s") + new Duration("PT1s"), + false ) ); } diff --git a/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java b/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java index fef244a62961..f2a95c692f8f 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java @@ -32,6 +32,7 @@ public class TestDruidCoordinatorConfig extends DruidCoordinatorConfig private final Duration coordinatorKillDurationToRetain; private final Duration getLoadQueuePeonRepeatDelay; private final int coordinatorKillMaxSegments; + private final boolean logUsedSegmentsDutyEnabled; public TestDruidCoordinatorConfig( Duration coordinatorStartDelay, @@ -41,7 +42,8 @@ public TestDruidCoordinatorConfig( Duration coordinatorKillPeriod, Duration coordinatorKillDurationToRetain, int coordinatorKillMaxSegments, - Duration getLoadQueuePeonRepeatDelay + Duration getLoadQueuePeonRepeatDelay, + boolean logUsedSegmentsDutyEnabled ) { this.coordinatorStartDelay = coordinatorStartDelay; @@ -52,6 +54,7 @@ public TestDruidCoordinatorConfig( this.coordinatorKillDurationToRetain = coordinatorKillDurationToRetain; this.coordinatorKillMaxSegments = coordinatorKillMaxSegments; this.getLoadQueuePeonRepeatDelay = getLoadQueuePeonRepeatDelay; + this.logUsedSegmentsDutyEnabled = logUsedSegmentsDutyEnabled; } @Override @@ -96,6 +99,12 @@ public Duration getLoadTimeoutDelay() return loadTimeoutDelay == null ? super.getLoadTimeoutDelay() : loadTimeoutDelay; } + @Override + public boolean isLogUsedSegmentsDutyEnabled() + { + return logUsedSegmentsDutyEnabled; + } + @Override public Duration getLoadQueuePeonRepeatDelay() { diff --git a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java index 38aa78e4467c..0dd2aa63adf0 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java @@ -109,7 +109,8 @@ private void testFindIntervalForKill(List segmentIntervals, Interval e Duration.parse("PT86400S"), Duration.parse("PT86400S"), 1000, - Duration.ZERO + Duration.ZERO, + false ) ); From 4b9a54b7aea2d585626649890480e19c0b5636bd Mon Sep 17 00:00:00 2001 From: Lucas Capistrant Date: Sat, 15 Aug 2020 16:56:25 -0500 Subject: [PATCH 2/8] Fixes for TravisCI coverage checks and documentation spell checking --- docs/configuration/index.md | 2 +- .../druid/server/coordinator/CuratorDruidCoordinatorTest.java | 2 +- .../apache/druid/server/coordinator/DruidCoordinatorTest.java | 2 +- .../druid/server/coordinator/HttpLoadQueuePeonTest.java | 2 +- .../apache/druid/server/coordinator/LoadQueuePeonTest.java | 4 ++-- .../apache/druid/server/coordinator/LoadQueuePeonTester.java | 2 +- .../druid/server/coordinator/duty/KillUnusedSegmentsTest.java | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 0e5375f6482d..87768e51963b 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -689,7 +689,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| -|`druid.coordinator.duties.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the LogUsedSegments Duty|true| +|`druid.coordinator.duties.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` Duty|true| ##### Segment Management |Property|Possible Values|Description|Default| diff --git a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java index 28bae21a6518..c62600562ba2 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java @@ -172,7 +172,7 @@ public void setUp() throws Exception null, 10, new Duration("PT0s"), - false + true ); sourceLoadQueueChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java index 7886cd851fae..c52e85316f0d 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java @@ -143,7 +143,7 @@ public void setUp() throws Exception null, 10, new Duration("PT0s"), - false + true ); pathChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java index 97e72eda3273..4234bb1a9884 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java @@ -82,7 +82,7 @@ public class HttpLoadQueuePeonTest null, 10, Duration.ZERO, - false + true ) { @Override diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java index 7e30c0d1f8f7..a4d7928c7929 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java @@ -97,7 +97,7 @@ public void testMultipleLoadDropSegments() throws Exception null, 10, Duration.millis(0), - false + true ) ); @@ -292,7 +292,7 @@ public void testFailAssign() throws Exception null, 10, new Duration("PT1s"), - false + true ) ); diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java index e101af8ee28b..5cc1a098f447 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java @@ -46,7 +46,7 @@ public LoadQueuePeonTester() null, 10, new Duration("PT1s"), - false + true ) ); } diff --git a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java index 0dd2aa63adf0..e6645057447b 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java @@ -110,7 +110,7 @@ private void testFindIntervalForKill(List segmentIntervals, Interval e Duration.parse("PT86400S"), 1000, Duration.ZERO, - false + true ) ); From e16219b79407b31fb9b33a3d8c5f6784f41953d3 Mon Sep 17 00:00:00 2001 From: Lucas Capistrant Date: Sun, 16 Aug 2020 13:11:31 -0500 Subject: [PATCH 3/8] prameterize DruidCoordinatorTest in order to achieve coverage --- .../coordinator/DruidCoordinatorTest.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java index c52e85316f0d..266fa12e850c 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java @@ -65,8 +65,11 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; import javax.annotation.Nullable; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.Map; @@ -79,6 +82,7 @@ /** */ +@RunWith(Parameterized.class) public class DruidCoordinatorTest extends CuratorTestBase { private static final String LOADPATH = "/druid/loadqueue/localhost:1234"; @@ -104,6 +108,24 @@ public class DruidCoordinatorTest extends CuratorTestBase private DruidNode druidNode; private LatchableServiceEmitter serviceEmitter = new LatchableServiceEmitter(); + private boolean logUsedSegmentsDutyEnabled; + + public DruidCoordinatorTest(boolean logUsedSegmentsDutyEnabled) + { + this.logUsedSegmentsDutyEnabled = logUsedSegmentsDutyEnabled; + } + + @Parameterized.Parameters(name = "{index}: logUsedSegmentsDutyEnabled:{0}") + public static Iterable data() + { + return Arrays.asList( + new Object[][]{ + {false}, + {true} + } + ); + } + @Before public void setUp() throws Exception { @@ -143,7 +165,7 @@ public void setUp() throws Exception null, 10, new Duration("PT0s"), - true + logUsedSegmentsDutyEnabled ); pathChildrenCache = new PathChildrenCache( curator, From d6cf0ec9ad2db4a62b4b5a6b846b4f5bdf4748c4 Mon Sep 17 00:00:00 2001 From: "Lucas.Capistrant" Date: Wed, 28 Oct 2020 19:06:16 -0500 Subject: [PATCH 4/8] update config name to remove duty ref and improve documentation --- docs/configuration/index.md | 2 +- .../apache/druid/server/coordinator/DruidCoordinatorConfig.java | 2 +- .../druid/server/coordinator/DruidCoordinatorConfigTest.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 09ce2e91cb8c..ee4812b485ea 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -698,7 +698,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| -|`druid.coordinator.duties.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` Duty|true| +|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle. It gets a snapshot of segments in the cluster and iterates them. While iterating, it will emit an alert if a segment has a size less than 0. If debug logging is enabled, it will also log a string representation of each segment. Lastly, it logs then number of segments in the cluster. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.|true| ##### Segment Management |Property|Possible Values|Description|Default| diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java index 3b24da2d7fc8..c219421ca594 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java @@ -51,7 +51,7 @@ public abstract class DruidCoordinatorConfig @Default("0") public abstract int getCoordinatorKillMaxSegments(); - @Config("druid.coordinator.duties.logUsedSegments.enabled") + @Config("druid.coordinator.logUsedSegments.enabled") @Default("true") public abstract boolean isLogUsedSegmentsDutyEnabled(); diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java index 9b8e4910fe42..5e315e74890a 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java @@ -61,7 +61,7 @@ public void testDeserialization() props.setProperty("druid.coordinator.kill.pendingSegments.on", "true"); props.setProperty("druid.coordinator.load.timeout", "PT1s"); props.setProperty("druid.coordinator.loadqueuepeon.repeatDelay", "PT0.100s"); - props.setProperty("druid.coordinator.duties.logUsedSegments.enabled", "false"); + props.setProperty("druid.coordinator.logUsedSegments.enabled", "false"); factory = Config.createFactory(props); config = factory.build(DruidCoordinatorConfig.class); From 60f2067c2b04d75d5e260c4aef0c6009e31a088d Mon Sep 17 00:00:00 2001 From: "Lucas.Capistrant" Date: Thu, 29 Oct 2020 08:27:42 -0500 Subject: [PATCH 5/8] refine documentation for new config with reviewer advice --- docs/configuration/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index ee4812b485ea..5467aa53f819 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -698,7 +698,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| -|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle. It gets a snapshot of segments in the cluster and iterates them. While iterating, it will emit an alert if a segment has a size less than 0. If debug logging is enabled, it will also log a string representation of each segment. Lastly, it logs then number of segments in the cluster. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.|true| +|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle which logs every segment at DEBUG level and the total number of used segments at INFO level. In addition to these logs, it emits an alert if a segment has a size less than 0. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.| ##### Segment Management |Property|Possible Values|Description|Default| From 20bd6e244827ca5523ca0504360ffa2aefad6bb8 Mon Sep 17 00:00:00 2001 From: "Lucas.Capistrant" Date: Tue, 5 Jan 2021 16:08:05 -0600 Subject: [PATCH 6/8] add default column to docs for new config --- docs/configuration/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 5467aa53f819..853e8bc85411 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -698,7 +698,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| -|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle which logs every segment at DEBUG level and the total number of used segments at INFO level. In addition to these logs, it emits an alert if a segment has a size less than 0. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.| +|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle which logs every segment at DEBUG level and the total number of used segments at INFO level. In addition to these logs, it emits an alert if a segment has a size less than 0. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.|false| ##### Segment Management |Property|Possible Values|Description|Default| From e77b3209ec5b068d19d3f04c448489c686986942 Mon Sep 17 00:00:00 2001 From: "Lucas.Capistrant" Date: Fri, 8 Jan 2021 12:24:59 -0600 Subject: [PATCH 7/8] remove legacy code in LogUsedSegments and remove config to disbale duty --- docs/configuration/index.md | 1 - .../server/coordinator/DruidCoordinator.java | 8 ++---- .../coordinator/DruidCoordinatorConfig.java | 4 --- .../coordinator/duty/LogUsedSegments.java | 12 ++------- .../CuratorDruidCoordinatorTest.java | 3 +-- .../DruidCoordinatorConfigTest.java | 3 --- .../coordinator/DruidCoordinatorTest.java | 25 +------------------ .../coordinator/HttpLoadQueuePeonTest.java | 3 +-- .../server/coordinator/LoadQueuePeonTest.java | 6 ++--- .../coordinator/LoadQueuePeonTester.java | 3 +-- .../TestDruidCoordinatorConfig.java | 11 +------- .../duty/KillUnusedSegmentsTest.java | 3 +-- 12 files changed, 12 insertions(+), 70 deletions(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 20a5e54cd344..657b652b466a 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -698,7 +698,6 @@ These Coordinator static configurations can be defined in the `coordinator/runti |`druid.coordinator.loadqueuepeon.repeatDelay`|The start and repeat delay for the loadqueuepeon, which manages the load and drop of segments.|PT0.050S (50 ms)| |`druid.coordinator.asOverlord.enabled`|Boolean value for whether this Coordinator process should act like an Overlord as well. This configuration allows users to simplify a druid cluster by not having to deploy any standalone Overlord processes. If set to true, then Overlord console is available at `http://coordinator-host:port/console.html` and be sure to set `druid.coordinator.asOverlord.overlordService` also. See next.|false| |`druid.coordinator.asOverlord.overlordService`| Required, if `druid.coordinator.asOverlord.enabled` is `true`. This must be same value as `druid.service` on standalone Overlord processes and `druid.selectors.indexing.serviceName` on Middle Managers.|NULL| -|`druid.coordinator.logUsedSegments.enabled`|Boolean value for whether or not the coordinator should execute the `LogUsedSegments` portion of its coordination work. `LogUsedSegments` is an informational job run by the Coordinator every coordination cycle which logs every segment at DEBUG level and the total number of used segments at INFO level. In addition to these logs, it emits an alert if a segment has a size less than 0. An admin can decide that forgoing this work may advantageous if they don't need any of the information provided.|false| ##### Segment Management |Property|Possible Values|Description|Default| diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java index 78f352fb0341..af862e4acccb 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java @@ -677,10 +677,8 @@ private void stopBeingLeader() private List makeHistoricalManagementDuties() { List duties = new ArrayList<>(); - if (config.isLogUsedSegmentsDutyEnabled()) { - duties.add(new LogUsedSegments()); - } duties.addAll(ImmutableList.of( + new LogUsedSegments(), new UpdateCoordinatorStateAndPrepareCluster(), new RunRules(DruidCoordinator.this), new UnloadUnusedSegments(), @@ -698,9 +696,7 @@ private List makeHistoricalManagementDuties() private List makeIndexingServiceDuties() { List duties = new ArrayList<>(); - if (config.isLogUsedSegmentsDutyEnabled()) { - duties.add(new LogUsedSegments()); - } + duties.add(new LogUsedSegments()); duties.addAll(indexingServiceDuties); // CompactSegmentsDuty should be the last duty as it can take a long time to complete duties.addAll(makeCompactSegmentsDuty()); diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java index c219421ca594..130d7e88d840 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinatorConfig.java @@ -51,10 +51,6 @@ public abstract class DruidCoordinatorConfig @Default("0") public abstract int getCoordinatorKillMaxSegments(); - @Config("druid.coordinator.logUsedSegments.enabled") - @Default("true") - public abstract boolean isLogUsedSegmentsDutyEnabled(); - @Config("druid.coordinator.load.timeout") public Duration getLoadTimeoutDelay() { diff --git a/server/src/main/java/org/apache/druid/server/coordinator/duty/LogUsedSegments.java b/server/src/main/java/org/apache/druid/server/coordinator/duty/LogUsedSegments.java index 03c814d944d6..3904db9ecd57 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/duty/LogUsedSegments.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/duty/LogUsedSegments.java @@ -37,17 +37,9 @@ public DruidCoordinatorRuntimeParams run(DruidCoordinatorRuntimeParams params) { log.debug("Starting coordination. Getting used segments."); - DataSourcesSnapshot dataSourcesSnapshot = params.getDataSourcesSnapshot(); - for (DataSegment segment : dataSourcesSnapshot.iterateAllUsedSegmentsInSnapshot()) { - if (segment.getSize() < 0) { - log.makeAlert("No size on a segment") - .addData("segment", segment) - .emit(); - } - } - - // Log info about all used segments + // Log info about all used segments only if debug logging is enabled if (log.isDebugEnabled()) { + DataSourcesSnapshot dataSourcesSnapshot = params.getDataSourcesSnapshot(); log.debug("Used Segments"); for (DataSegment dataSegment : dataSourcesSnapshot.iterateAllUsedSegmentsInSnapshot()) { log.debug(" %s", dataSegment); diff --git a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java index 34dbc5f89929..dd382e31a65f 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/CuratorDruidCoordinatorTest.java @@ -172,8 +172,7 @@ public void setUp() throws Exception new Duration(COORDINATOR_PERIOD), null, 10, - new Duration("PT0s"), - true + new Duration("PT0s") ); sourceLoadQueueChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java index 5e315e74890a..ad563410fe61 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorConfigTest.java @@ -47,7 +47,6 @@ public void testDeserialization() Assert.assertEquals(0, config.getCoordinatorKillMaxSegments()); Assert.assertEquals(new Duration(15 * 60 * 1000), config.getLoadTimeoutDelay()); Assert.assertEquals(Duration.millis(50), config.getLoadQueuePeonRepeatDelay()); - Assert.assertTrue(config.isLogUsedSegmentsDutyEnabled()); //with non-defaults Properties props = new Properties(); @@ -61,7 +60,6 @@ public void testDeserialization() props.setProperty("druid.coordinator.kill.pendingSegments.on", "true"); props.setProperty("druid.coordinator.load.timeout", "PT1s"); props.setProperty("druid.coordinator.loadqueuepeon.repeatDelay", "PT0.100s"); - props.setProperty("druid.coordinator.logUsedSegments.enabled", "false"); factory = Config.createFactory(props); config = factory.build(DruidCoordinatorConfig.class); @@ -74,6 +72,5 @@ public void testDeserialization() Assert.assertEquals(10000, config.getCoordinatorKillMaxSegments()); Assert.assertEquals(new Duration("PT1s"), config.getLoadTimeoutDelay()); Assert.assertEquals(Duration.millis(100), config.getLoadQueuePeonRepeatDelay()); - Assert.assertFalse(config.isLogUsedSegmentsDutyEnabled()); } } diff --git a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java index b210c35afe6f..31bfeeccc77a 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/DruidCoordinatorTest.java @@ -67,11 +67,8 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; import javax.annotation.Nullable; -import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.Map; @@ -84,7 +81,6 @@ /** */ -@RunWith(Parameterized.class) public class DruidCoordinatorTest extends CuratorTestBase { private static final String LOADPATH = "/druid/loadqueue/localhost:1234"; @@ -110,24 +106,6 @@ public class DruidCoordinatorTest extends CuratorTestBase private DruidNode druidNode; private LatchableServiceEmitter serviceEmitter = new LatchableServiceEmitter(); - private boolean logUsedSegmentsDutyEnabled; - - public DruidCoordinatorTest(boolean logUsedSegmentsDutyEnabled) - { - this.logUsedSegmentsDutyEnabled = logUsedSegmentsDutyEnabled; - } - - @Parameterized.Parameters(name = "{index}: logUsedSegmentsDutyEnabled:{0}") - public static Iterable data() - { - return Arrays.asList( - new Object[][]{ - {false}, - {true} - } - ); - } - @Before public void setUp() throws Exception { @@ -166,8 +144,7 @@ public void setUp() throws Exception new Duration(COORDINATOR_PERIOD), null, 10, - new Duration("PT0s"), - logUsedSegmentsDutyEnabled + new Duration("PT0s") ); pathChildrenCache = new PathChildrenCache( curator, diff --git a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java index 4234bb1a9884..10d7ba24c40e 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/HttpLoadQueuePeonTest.java @@ -81,8 +81,7 @@ public class HttpLoadQueuePeonTest null, null, 10, - Duration.ZERO, - true + Duration.ZERO ) { @Override diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java index 619fec76a81e..90202648186f 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTest.java @@ -96,8 +96,7 @@ public void testMultipleLoadDropSegments() throws Exception null, null, 10, - Duration.millis(0), - true + Duration.millis(0) ) ); @@ -292,8 +291,7 @@ public void testFailAssign() throws Exception null, null, 10, - new Duration("PT1s"), - true + new Duration("PT1s") ) ); diff --git a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java index 5cc1a098f447..fb9287320dca 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/LoadQueuePeonTester.java @@ -45,8 +45,7 @@ public LoadQueuePeonTester() null, null, 10, - new Duration("PT1s"), - true + new Duration("PT1s") ) ); } diff --git a/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java b/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java index f2a95c692f8f..fef244a62961 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/TestDruidCoordinatorConfig.java @@ -32,7 +32,6 @@ public class TestDruidCoordinatorConfig extends DruidCoordinatorConfig private final Duration coordinatorKillDurationToRetain; private final Duration getLoadQueuePeonRepeatDelay; private final int coordinatorKillMaxSegments; - private final boolean logUsedSegmentsDutyEnabled; public TestDruidCoordinatorConfig( Duration coordinatorStartDelay, @@ -42,8 +41,7 @@ public TestDruidCoordinatorConfig( Duration coordinatorKillPeriod, Duration coordinatorKillDurationToRetain, int coordinatorKillMaxSegments, - Duration getLoadQueuePeonRepeatDelay, - boolean logUsedSegmentsDutyEnabled + Duration getLoadQueuePeonRepeatDelay ) { this.coordinatorStartDelay = coordinatorStartDelay; @@ -54,7 +52,6 @@ public TestDruidCoordinatorConfig( this.coordinatorKillDurationToRetain = coordinatorKillDurationToRetain; this.coordinatorKillMaxSegments = coordinatorKillMaxSegments; this.getLoadQueuePeonRepeatDelay = getLoadQueuePeonRepeatDelay; - this.logUsedSegmentsDutyEnabled = logUsedSegmentsDutyEnabled; } @Override @@ -99,12 +96,6 @@ public Duration getLoadTimeoutDelay() return loadTimeoutDelay == null ? super.getLoadTimeoutDelay() : loadTimeoutDelay; } - @Override - public boolean isLogUsedSegmentsDutyEnabled() - { - return logUsedSegmentsDutyEnabled; - } - @Override public Duration getLoadQueuePeonRepeatDelay() { diff --git a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java index e6645057447b..38aa78e4467c 100644 --- a/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java +++ b/server/src/test/java/org/apache/druid/server/coordinator/duty/KillUnusedSegmentsTest.java @@ -109,8 +109,7 @@ private void testFindIntervalForKill(List segmentIntervals, Interval e Duration.parse("PT86400S"), Duration.parse("PT86400S"), 1000, - Duration.ZERO, - true + Duration.ZERO ) ); From c6d3ddf59f619079ef89c8a3eb27293bf6b11e12 Mon Sep 17 00:00:00 2001 From: "Lucas.Capistrant" Date: Fri, 8 Jan 2021 12:31:03 -0600 Subject: [PATCH 8/8] fix makeHistoricalMangementDuties now that the returned list is always the same --- .../apache/druid/server/coordinator/DruidCoordinator.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java index af862e4acccb..8414e50e1f1f 100644 --- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java +++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java @@ -676,8 +676,7 @@ private void stopBeingLeader() private List makeHistoricalManagementDuties() { - List duties = new ArrayList<>(); - duties.addAll(ImmutableList.of( + return ImmutableList.of( new LogUsedSegments(), new UpdateCoordinatorStateAndPrepareCluster(), new RunRules(DruidCoordinator.this), @@ -685,12 +684,7 @@ private List makeHistoricalManagementDuties() new MarkAsUnusedOvershadowedSegments(DruidCoordinator.this), new BalanceSegments(DruidCoordinator.this), new EmitClusterStatsAndMetrics(DruidCoordinator.this) - )); - log.debug( - "Done making historical management duties %s", - duties.stream().map(duty -> duty.getClass().getName()).collect(Collectors.toList()) ); - return duties; } private List makeIndexingServiceDuties()