abhishekrb19 commented on code in PR #15705:
URL: https://github.com/apache/druid/pull/15705#discussion_r1576813453
##########
server/src/test/java/org/apache/druid/server/coordinator/loading/LoadQueuePeonTest.java:
##########
@@ -321,12 +313,8 @@ public void testFailAssignForLoadDropTimeout() throws
Exception
Execs.singleThreaded("test_load_queue_peon-%d"),
// The timeout here was set to 1ms, when this test was acting flakey.
A cursory glance makes me wonder if
// there's a race where the timeout actually happens before other code
can run. 1ms timeout seems aggressive.
- // 100ms is a great price to pay if it removes the flakeyness
- new TestDruidCoordinatorConfig.Builder()
- .withLoadTimeoutDelay(new Duration(100))
- .withCoordinatorKillMaxSegments(10)
- .withCoordinatorKillIgnoreDurationToRetain(false)
- .build()
+ // 100ms is a great price to pay if it removes the flakeyness,
Review Comment:
;)
```suggestion
// 100ms is a great price to pay if it removes the flakiness,
```
##########
server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.config;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.common.config.Configs;
+import org.joda.time.Duration;
+
+public class CoordinatorKillConfigs
+{
+ public static CoordinatorKillConfigs STANDARD
Review Comment:
Calling it "default" would be more straightforward to understand than
"standard". What do you think?
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillSupervisorsCustomDuty.java:
##########
@@ -38,43 +37,32 @@
* <p>
* This duty is only an example to demostrate the usage of coordinator custom
Review Comment:
```suggestion
* This duty is only an example to demonstrate the usage of coordinator
custom
```
##########
server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.config;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.common.config.Configs;
+import org.joda.time.Duration;
+
+public class CoordinatorKillConfigs
+{
+ public static CoordinatorKillConfigs STANDARD
+ = new CoordinatorKillConfigs(null, null, null, null, null, null, null,
null, null, null, null, null);
+
+ @JsonProperty("supervisor")
+ private final MetadataCleanupConfig supervisors;
+
+ @JsonProperty("audit")
+ private final MetadataCleanupConfig audit;
+
+ @JsonProperty("datasource")
+ private final MetadataCleanupConfig datasource;
+
+ @JsonProperty("rule")
+ private final MetadataCleanupConfig rules;
+
+ @JsonProperty("compaction")
+ private final MetadataCleanupConfig compaction;
+
+ @JsonProperty("pendingSegments")
+ private final MetadataCleanupConfig pendingSegments;
+
+ // Raw configs for killing unused segments
+ // These have been added as fields because KillUnusedSegmentsConfig is
initialized lazily
+ @JsonProperty("on")
+ private final Boolean killUnusedEnabled;
+
+ @JsonProperty("period")
+ private final Duration killUnusedPeriod;
+
+ @JsonProperty("durationToRetain")
+ private final Duration killUnusedDurationToRetain;
+
+ @JsonProperty("ignoreDurationToRetain")
+ private final Boolean killUnusedIgnoreDurationToRetain;
+
+ @JsonProperty("bufferPeriod")
+ private final Duration killUnusedBufferPeriod;
+
+ @JsonProperty("maxSegments")
+ private final Integer killUnusedMaxSegments;
+
+ @JsonCreator
+ public CoordinatorKillConfigs(
+ @JsonProperty("pendingSegments") MetadataCleanupConfig pendingSegments,
+ @JsonProperty("supervisor") MetadataCleanupConfig supervisors,
+ @JsonProperty("audit") MetadataCleanupConfig audit,
+ @JsonProperty("datasource") MetadataCleanupConfig datasource,
+ @JsonProperty("rule") MetadataCleanupConfig rules,
+ @JsonProperty("compaction") MetadataCleanupConfig compaction,
+ // Configs for cleanup of unused segments
Review Comment:
:( wish he had a namespace prefix for unused segments similar to the
others...
##########
server/src/main/java/org/apache/druid/server/coordinator/config/CoordinatorKillConfigs.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.config;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.common.config.Configs;
+import org.joda.time.Duration;
+
+public class CoordinatorKillConfigs
+{
+ public static CoordinatorKillConfigs STANDARD
+ = new CoordinatorKillConfigs(null, null, null, null, null, null, null,
null, null, null, null, null);
+
+ @JsonProperty("supervisor")
+ private final MetadataCleanupConfig supervisors;
+
+ @JsonProperty("audit")
+ private final MetadataCleanupConfig audit;
+
+ @JsonProperty("datasource")
+ private final MetadataCleanupConfig datasource;
+
+ @JsonProperty("rule")
+ private final MetadataCleanupConfig rules;
+
+ @JsonProperty("compaction")
+ private final MetadataCleanupConfig compaction;
+
+ @JsonProperty("pendingSegments")
+ private final MetadataCleanupConfig pendingSegments;
+
+ // Raw configs for killing unused segments
+ // These have been added as fields because KillUnusedSegmentsConfig is
initialized lazily
Review Comment:
```suggestion
/**
* Raw configs for killing unused segments. These have been added as fields
* because {@link KillUnusedSegmentsConfig} is initialized lazily.
*/
```
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillUnusedSegments.java:
##########
@@ -90,44 +88,25 @@ public class KillUnusedSegments implements CoordinatorDuty
private final OverlordClient overlordClient;
public KillUnusedSegments(
- final SegmentsMetadataManager segmentsMetadataManager,
- final OverlordClient overlordClient,
- final DruidCoordinatorConfig config
+ SegmentsMetadataManager segmentsMetadataManager,
+ OverlordClient overlordClient,
+ KillUnusedSegmentsConfig killConfig
)
{
- if (config.getCoordinatorKillPeriod().getMillis() <
config.getCoordinatorIndexingPeriod().getMillis()) {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.INVALID_INPUT)
- .build(
- StringUtils.format(
- "druid.coordinator.kill.period[%s] is
invalid. It must be greater than or "
- + "equal to
druid.coordinator.period.indexingPeriod[%s].",
- config.getCoordinatorKillPeriod(),
- config.getCoordinatorIndexingPeriod()
- )
- );
- }
- if (config.getCoordinatorKillMaxSegments() < 0) {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.INVALID_INPUT)
- .build(StringUtils.format(
- "druid.coordinator.kill.maxSegments[%d]
is invalid. It must be a positive integer.",
- config.getCoordinatorKillMaxSegments()
- )
- );
- }
- this.period = config.getCoordinatorKillPeriod();
- this.ignoreDurationToRetain =
config.getCoordinatorKillIgnoreDurationToRetain();
- this.durationToRetain = config.getCoordinatorKillDurationToRetain();
+ this.period = killConfig.getCleanupPeriod();
+
+ this.maxSegmentsToKill = killConfig.getMaxSegments();
+ this.ignoreDurationToRetain = killConfig.isIgnoreDurationToRetain();
+ this.durationToRetain = killConfig.getDurationToRetain();
if (this.ignoreDurationToRetain) {
log.info(
"druid.coordinator.kill.durationToRetain[%s] will be ignored when
discovering segments to kill "
+ "because druid.coordinator.kill.ignoreDurationToRetain is set to
true.",
- this.durationToRetain
+ durationToRetain
);
}
- this.bufferPeriod = config.getCoordinatorKillBufferPeriod().getMillis();
- this.maxSegmentsToKill = config.getCoordinatorKillMaxSegments();
+ this.bufferPeriod = killConfig.getBufferPeriod().getMillis();
Review Comment:
nit: taking a fresh look, it looks like we can store the buffer period
directly as-is without the millis part (or rename it to `bufferPeriodMillis` to
avoid ambiguity)
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/KillSupervisorsCustomDuty.java:
##########
@@ -38,43 +37,32 @@
* <p>
* This duty is only an example to demostrate the usage of coordinator custom
* duties. All production clusters should continue using {@link
KillSupervisors}.
- * <p>
- * In the future, we might migrate all metadata management coordinator duties
to
- * {@link CoordinatorCustomDuty} but until then this class will remain
undocumented.
*/
@UnstableApi
-public class KillSupervisorsCustomDuty extends MetadataCleanupDuty implements
CoordinatorCustomDuty
+public class KillSupervisorsCustomDuty implements CoordinatorCustomDuty
{
private static final Logger log = new
Logger(KillSupervisorsCustomDuty.class);
- private final MetadataSupervisorManager metadataSupervisorManager;
+ private final KillSupervisors delegate;
@JsonCreator
public KillSupervisorsCustomDuty(
@JsonProperty("durationToRetain") Duration retainDuration,
- @JacksonInject MetadataSupervisorManager metadataSupervisorManager,
- @JacksonInject DruidCoordinatorConfig coordinatorConfig
+ @JacksonInject MetadataSupervisorManager metadataSupervisorManager
)
{
- super(
- "supervisors",
- "KillSupervisorsCustomDuty",
- true,
- // Use the same period as metadata store management so that validation
passes
- // Actual period of custom duties is configured by the user
- coordinatorConfig.getCoordinatorMetadataStoreManagementPeriod(),
- retainDuration,
- Stats.Kill.SUPERVISOR_SPECS,
- coordinatorConfig
+ this.delegate = new KillSupervisors(
+ // Pass period as zero here, actual period of custom duties is
configured at the duty group level
+ new MetadataCleanupConfig(true, Duration.ZERO, retainDuration),
+ metadataSupervisorManager
);
- this.metadataSupervisorManager = metadataSupervisorManager;
log.warn("This is only an example implementation of a custom duty and"
Review Comment:
I suppose if the delegate is `KillSupervisors`, this log is not accurate
anymore?
Also, curious, if this is meant to be an example implementation of a custom
duty, should this code just reside in a separate tree in the repo?
##########
server/src/main/java/org/apache/druid/server/coordinator/config/MetadataCleanupConfig.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.config;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.druid.common.config.Configs;
+import org.joda.time.Duration;
+
+import java.util.Objects;
+
+public class MetadataCleanupConfig
+{
+ public static final MetadataCleanupConfig STANDARD = new
MetadataCleanupConfig(null, null, null);
Review Comment:
`DEFAULT` here as well.
--
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]
