suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int
DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1;
// Disabled
private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
= 60 * 60; // 1 Hour.
+ private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
=
+ 60 * 60; // 1 Hour.
private static final int
DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS =
60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS =
"server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD =
"server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should
move towards taking everything as human-readable strings and converting it to
seconds (to keep it generic, basically if user wanted to keep the timeout as
2m, he should be able to).
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static
org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS,
BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS,
STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS,
TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS,
MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+
MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS,
SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS,
SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD,
OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD,
REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD,
BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD,
STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+
MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD,
TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD,
SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD,
OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period
config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config,
DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config,
DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is
returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config,
DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value
is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config,
DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an
{@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
That's a valid point @mcvsubbu. I just went with the simplest approach
as the test was very trivial, and you pretty much can point where exactly the
exception was thrown. Will change it.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int
DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1;
// Disabled
private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
= 60 * 60; // 1 Hour.
+ private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
=
+ 60 * 60; // 1 Hour.
private static final int
DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS =
60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS =
"server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD =
"server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should
move towards taking everything as human-readable strings and converting it to
whatever units it wants to (to keep it generic, basically if user wanted to
keep the timeout as 2m, he should be able to. The config takes care to changing
it to seconds). Will address this.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
"The behavior should be that the new one should be used if both are
specified or only the new one be specified" - Isn't it what it is doing
@jackjlli? Basically if the new config is absent, it falls-back to the older
one. But method still does what it says - `getSegmentCommitTimeoutSeconds`.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int
retentionFrequencyInSec
}
/**
- * Returns the config value for
controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This
is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level
validation interval frequency
- * The default value is the new
DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for
controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
Missed this, will update the docs.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int
DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1;
// Disabled
private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
= 60 * 60; // 1 Hour.
+ private static final int
DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS
=
+ 60 * 60; // 1 Hour.
private static final int
DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS =
60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS =
"server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD =
"server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should
move towards taking everything as human-readable strings and converting it to
whatever units it wants to (to keep it generic, basically if user wanted to
keep the timeout as 2m, he should be able to. The config takes care to changing
of to seconds). Will address 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]
