You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2024/01/17 11:38:22 UTC

[PR] Remove usage of skife from DruidCoordinatorConfig (druid)

kfaraz opened a new pull request, #15705:
URL: https://github.com/apache/druid/pull/15705

   [WIP: Do Not Merge]
   
   ### Description
   
   This is a follow up to #14695 . While the first PR addressed `DruidProcessingConfig`, this PR tries to get rid of skife configs from `DruidCoordinatorConfig`.
   
   ### Changes
   - Add the following new config classes. More details about the bound paths in the next section.
     - `CoordinatorKillConfigs`
     - `CoordinatorRunConfig`
     - `CoordinatorPeriodConfig`
     - `MetadataCleanupConfig`
   - `DruidCoordinatorConfig` is not directly bound to any config path anymore. Instead, all the bound configs get injected into it.
   - 
   
   
   ### Bound config paths
   
   ### Release note
   TODO
   
   <hr>
   
   This PR has:
   
   - [ ] been self-reviewed.
      - [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] a release note entry in the PR description.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
   - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [ ] been tested in a test Druid cluster.
   


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

Re: [PR] Remove usage of skife from DruidCoordinatorConfig (druid)

Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.

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