xiangfu0 commented on code in PR #18001:
URL: https://github.com/apache/pinot/pull/18001#discussion_r3003426136
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -761,27 +715,25 @@ public void
setRealtimeOffsetAutoResetBackfillFrequencyInSeconds(int offsetAutoR
}
Review Comment:
Good catch. Fixed `setRealtimeOffsetAutoResetBackfillFrequencyInSeconds()`
to append `"s"` suffix so it writes a valid period string. See commit 1f1d5df.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -831,7 +784,7 @@ public long
getTenantRebalanceCheckerInitialDelayInSeconds() {
public int getRealtimeConsumerMonitorRunFrequency() {
return
Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.RT_CONSUMER_MONITOR_FREQUENCY_PERIOD))
.map(period -> (int) convertPeriodToSeconds(period))
-
.orElse(ControllerPeriodicTasksConf.DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_IN_SECONDS);
+ .orElse((int)
convertPeriodToSeconds(ControllerPeriodicTasksConf.DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_PERIOD));
Review Comment:
Done. Added the missing `DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_PERIOD =
"-1s"` constant (disabled by default). See commit 1f1d5df.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -687,72 +640,73 @@ 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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+ .orElse((int)
convertPeriodToSeconds(ControllerPeriodicTasksConf.DEFAULT_RETENTION_MANAGER_FREQUENCY_PERIOD));
}
public void setRetentionControllerFrequencyInSeconds(int
retentionFrequencyInSeconds) {
-
setProperty(ControllerPeriodicTasksConf.DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
- Integer.toString(retentionFrequencyInSeconds));
+ setProperty(ControllerPeriodicTasksConf.RETENTION_MANAGER_FREQUENCY_PERIOD,
+ Long.toString(retentionFrequencyInSeconds) + "s");
}
/**
- * Returns
<code>controller.offline.segment.interval.checker.frequencyPeriod</code>, or
- * <code>controller.offline.segment.interval.checker.frequencyPeriod</code>
or the segment level
- * validation interval, in the order of decreasing preference from left to
right. Falls-back to
- * the next config only if the current config is missing. 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 offline segment interval checker frequency in seconds.
+ * Reads {@code controller.offline.segment.interval.checker.frequencyPeriod}
as a period string (e.g. "24h"),
+ * falling back to {@code
DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD} if absent or invalid.
*
- * @return the supplied config in seconds
+ * @return the configured frequency in seconds
*/
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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+ .orElse(
+ (int) convertPeriodToSeconds(
+
ControllerPeriodicTasksConf.DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD));
Review Comment:
Done. Simplified all getters to use `getProperty(key, default)` pattern
instead of Optional chains. The period default is now passed directly as the
default value. See commit 1f1d5df.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java:
##########
@@ -292,6 +198,16 @@ private String getRandomString() {
return RandomStringUtils.randomAlphanumeric(5);
}
Review Comment:
Done. Removed dead `getRandomString()` and `getRandomDurationInSeconds()`
methods, and the unused `RandomStringUtils` import. See commit 1f1d5df.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -687,63 +640,54 @@ 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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+
.orElse(ControllerPeriodicTasksConf.DEFAULT_RETENTION_MANAGER_FREQUENCY_IN_SECONDS);
Review Comment:
Done. Converted all `DEFAULT_*_IN_SECONDS` int constants to
`DEFAULT_*_PERIOD` string constants (e.g. `"6h"`, `"1h"`, `"-1s"`). See commit
8ffa003.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -687,14 +640,13 @@ 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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+
.orElse(ControllerPeriodicTasksConf.DEFAULT_RETENTION_MANAGER_FREQUENCY_IN_SECONDS);
Review Comment:
Done. Added `invalidFrequencyPeriodShouldFallBackToDefault()` test in
ControllerConfTest that verifies invalid period values fall back to the
documented default. See commit 21973a2.
--
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]
