Jackie-Jiang commented on code in PR #15466:
URL: https://github.com/apache/pinot/pull/15466#discussion_r2072235191
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -367,6 +367,8 @@ private static long getRandomInitialDelayInSeconds() {
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 HashMap<>();
Review Comment:
We need a concurrent map here for thread safety
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -1049,12 +1078,15 @@ public boolean isHybridTableRetentionStrategyEnabled() {
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));
+ }
Review Comment:
(minor) Indentation seems wrong
##########
pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java:
##########
@@ -88,9 +88,10 @@ public void
validNewConfigOverridesCorrespondingValidOrInvalidOldConfigOnRead()
* 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()
{
Review Comment:
Can we also add a test for the invalid configs map value?
--
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]
