This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new cba670fa4f Fallback for bad values in periodic controller task
configurations (#15466)
cba670fa4f is described below
commit cba670fa4fcd9a82fb6155563b9ea25ff0ad668b
Author: ayesheepatra07 <ayeshee.pa...@startree.ai>
AuthorDate: Tue May 6 16:13:53 2025 -0700
Fallback for bad values in periodic controller task configurations (#15466)
---
.../apache/pinot/controller/ControllerConf.java | 62 ++++++++++++++++++++--
.../api/resources/PinotControllerHealthCheck.java | 16 ++++++
.../pinot/controller/ControllerConfTest.java | 22 ++++++--
3 files changed, 92 insertions(+), 8 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
index d405c4dcf0..55728b0fc9 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
@@ -25,6 +25,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Random;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import org.apache.commons.configuration2.Configuration;
import org.apache.commons.lang3.StringUtils;
@@ -376,6 +377,8 @@ public class ControllerConf extends PinotConfiguration {
public static final String EXIT_ON_SCHEMA_CHECK_FAILURE =
"controller.startup.exitOnSchemaCheckFailure";
public static final boolean DEFAULT_EXIT_ON_SCHEMA_CHECK_FAILURE = true;
+ private final Map<String, String> _invalidConfigs = new
ConcurrentHashMap<>();
+
public ControllerConf() {
super(new HashMap<>());
}
@@ -602,6 +605,8 @@ public class ControllerConf extends PinotConfiguration {
public int getRetentionControllerFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.RETENTION_MANAGER_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+ ControllerPeriodicTasksConf.RETENTION_MANAGER_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_RETENTION_MANAGER_FREQUENCY_IN_SECONDS));
@@ -625,6 +630,8 @@ public class ControllerConf extends PinotConfiguration {
public int getOfflineSegmentIntervalCheckerFrequencyInSeconds() {
return Optional.ofNullable(
getProperty(ControllerPeriodicTasksConf.OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(() ->
getProperty(
ControllerPeriodicTasksConf.DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS));
@@ -647,6 +654,8 @@ public class ControllerConf extends PinotConfiguration {
*/
public int getRealtimeSegmentValidationFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS));
@@ -669,6 +678,8 @@ public class ControllerConf extends PinotConfiguration {
*/
public int getBrokerResourceValidationFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS));
@@ -686,6 +697,7 @@ public class ControllerConf extends PinotConfiguration {
public int getStatusCheckerFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.STATUS_CHECKER_FREQUENCY_PERIOD))
+ .filter(period ->
isValidPeriodWithLogging(ControllerPeriodicTasksConf.STATUS_CHECKER_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_STATUS_CHECKER_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_STATUS_CHECKER_FREQUENCY_IN_SECONDS));
@@ -698,6 +710,8 @@ public class ControllerConf extends PinotConfiguration {
public int getRebalanceCheckerFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.REBALANCE_CHECKER_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+ ControllerPeriodicTasksConf.REBALANCE_CHECKER_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period))
.orElse(ControllerPeriodicTasksConf.DEFAULT_REBALANCE_CHECKER_FREQUENCY_IN_SECONDS);
}
@@ -720,6 +734,8 @@ public class ControllerConf extends PinotConfiguration {
public int getTaskMetricsEmitterFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_METRICS_EMITTER_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+ ControllerPeriodicTasksConf.TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS));
@@ -732,6 +748,8 @@ public class ControllerConf extends PinotConfiguration {
public int getStatusCheckerWaitForPushTimeInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS));
@@ -749,6 +767,8 @@ public class ControllerConf extends PinotConfiguration {
*/
public int getSegmentRelocatorFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+ ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(() -> {
Integer segmentRelocatorFreqSeconds =
getProperty(ControllerPeriodicTasksConf.DEPRECATED_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
Integer.class);
@@ -877,6 +897,8 @@ public class ControllerConf extends PinotConfiguration {
public int getTaskManagerFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+ ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD, period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_TASK_MANAGER_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS));
@@ -890,6 +912,8 @@ public class ControllerConf extends PinotConfiguration {
@Deprecated
public int getMinionInstancesCleanupTaskFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
() ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS,
ControllerPeriodicTasksConf.DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS));
@@ -917,6 +941,8 @@ public class ControllerConf extends PinotConfiguration {
public int
getMinionInstancesCleanupTaskMinOfflineTimeBeforeDeletionInSeconds() {
return Optional.ofNullable(
getProperty(ControllerPeriodicTasksConf.MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period)).orElseGet(() ->
getProperty(
ControllerPeriodicTasksConf.
DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
@@ -933,6 +959,8 @@ public class ControllerConf extends PinotConfiguration {
public int getStaleInstancesCleanupTaskFrequencyInSeconds() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.STALE_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.STALE_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period))
// Backward compatible for existing users who configured
MinionInstancesCleanupTask
.orElse(getMinionInstancesCleanupTaskFrequencyInSeconds());
@@ -955,6 +983,8 @@ public class ControllerConf extends PinotConfiguration {
public int getStaleInstancesCleanupTaskInstancesRetentionInSeconds() {
return Optional.ofNullable(
getProperty(ControllerPeriodicTasksConf.STALE_INSTANCES_CLEANUP_TASK_INSTANCES_RETENTION_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.STALE_INSTANCES_CLEANUP_TASK_INSTANCES_RETENTION_PERIOD,
period))
.map(period -> (int) convertPeriodToSeconds(period))
// Backward compatible for existing users who configured
MinionInstancesCleanupTask
.orElse(getMinionInstancesCleanupTaskMinOfflineTimeBeforeDeletionInSeconds());
@@ -1076,13 +1106,16 @@ public class ControllerConf extends PinotConfiguration {
return getProperty(ENABLE_HYBRID_TABLE_RETENTION_STRATEGY,
DEFAULT_ENABLE_HYBRID_TABLE_RETENTION_STRATEGY);
}
- public int getSegmentLevelValidationIntervalInSeconds() {
- return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD))
- .map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
- () ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
-
ControllerPeriodicTasksConf.DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS));
+ public int getSegmentLevelValidationIntervalInSeconds() {
+ return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD))
+ .filter(period -> isValidPeriodWithLogging(
+
ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD, period))
+ .map(period -> (int) convertPeriodToSeconds(period)).orElseGet(
+ () ->
getProperty(ControllerPeriodicTasksConf.DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
+
ControllerPeriodicTasksConf.DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS));
}
+
public boolean isAutoResetErrorSegmentsOnValidationEnabled() {
return
getProperty(ControllerPeriodicTasksConf.AUTO_RESET_ERROR_SEGMENTS_VALIDATION,
false);
}
@@ -1226,6 +1259,25 @@ public class ControllerConf extends PinotConfiguration {
return convertPeriodToUnit(period, TimeUnit.SECONDS);
}
+ private boolean isValidPeriodWithLogging(String propertyKey, String
periodStr) {
+ if (TimeUtils.isPeriodValid(periodStr)) {
+ return true;
+ } else {
+ addControllerInvalidConfigs(propertyKey,
+ String.format("Invalid time spec '%s' for config '%s'. Falling back
to default config.",
+ periodStr, propertyKey));
+ return false;
+ }
+ }
+
+ private void addControllerInvalidConfigs(String propertyKey, String
errorMessage) {
+ _invalidConfigs.put(propertyKey, errorMessage);
+ }
+
+ public Map<String, String> getInvalidConfigs() {
+ return _invalidConfigs;
+ }
+
private String getSupportedProtocol(String property) {
String value = getProperty(property, CommonConstants.HTTP_PROTOCOL);
Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(value),
"Unsupported %s protocol '%s'", property, value);
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java
index 61a18352ea..cf1de22181 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java
@@ -29,6 +29,8 @@ import io.swagger.annotations.SwaggerDefinition;
import java.time.Duration;
import java.time.Instant;
import java.time.format.DateTimeFormatter;
+import java.util.Collections;
+import java.util.Map;
import javax.inject.Inject;
import javax.inject.Named;
import javax.ws.rs.GET;
@@ -119,6 +121,20 @@ public class PinotControllerHealthCheck {
return uptime.getSeconds();
}
+ @GET
+ @Path("invalidconfigs")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_HEALTH)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List invalid controller configs")
+ public Response getConfigWarnings() {
+ Map<String, String> warnings = _controllerConf.getInvalidConfigs();
+ if (warnings.isEmpty()) {
+ return Response.ok(Collections.emptyMap()).build();
+ }
+
+ return Response.ok(warnings).build();
+ }
+
@GET
@Path("start-time")
@Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_HEALTH)
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
index c65a1b7a9f..120da8e8db 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
@@ -88,9 +88,10 @@ public class ControllerConfTest {
* are thrown when invalid new configurations are read (there is no
fall-back to the corresponding
* valid deprecated configuration). For all valid new configurations, they
override the
* corresponding deprecated configuration.
+ * Added fallback logic to use valid deprecated config when new config is
invalid.
*/
@Test
- public void
invalidNewConfigShouldThrowExceptionOnReadWithoutFallbackToCorrespondingValidDeprecatedConfig()
{
+ public void
invalidNewConfigShouldNotThrowExceptionOnReadWithFallbackToCorrespondingValidDeprecatedConfig()
{
//setup
Map<String, Object> controllerConfig = new HashMap<>();
int durationInSeconds = getRandomDurationInSeconds();
@@ -99,9 +100,24 @@ public class ControllerConfTest {
String randomPeriodInMinutes = getRandomPeriodInMinutes();
NEW_CONFIGS.forEach(config -> controllerConfig.put(config,
randomPeriodInMinutes));
//put some invalid new configs
- controllerConfig.put(RETENTION_MANAGER_FREQUENCY_PERIOD,
getRandomString());
+ String randomInvalidString = getRandomString();
+ controllerConfig.put(RETENTION_MANAGER_FREQUENCY_PERIOD,
randomInvalidString);
ControllerConf conf = new ControllerConf(controllerConfig);
- Assert.assertThrows(IllegalArgumentException.class,
conf::getRetentionControllerFrequencyInSeconds);
+ Assert.assertEquals(
+ conf.getRetentionControllerFrequencyInSeconds(),
+ durationInSeconds, // expected fallback value
+ "Should fallback to deprecated config value"
+ );
+ //test to assert that invalid config is captured in the invalid config map
value
+ Map<String, String> invalidConfigs = conf.getInvalidConfigs();
+
Assert.assertTrue(invalidConfigs.containsKey(RETENTION_MANAGER_FREQUENCY_PERIOD));
+ Assert.assertEquals(
+ conf.getInvalidConfigs().get(RETENTION_MANAGER_FREQUENCY_PERIOD),
+ String.format(
+ "Invalid time spec '%s' for config '%s'. Falling back to default
config.",
+ randomInvalidString, RETENTION_MANAGER_FREQUENCY_PERIOD
+ )
+ );
}
/**
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
