Copilot commented on code in PR #18001:
URL: https://github.com/apache/pinot/pull/18001#discussion_r3002628931
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -774,14 +724,13 @@ 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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+
.orElse(ControllerPeriodicTasksConf.DEFAULT_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS);
Review Comment:
The Javadoc above `getBrokerResourceValidationFrequencyInSeconds()` still
refers to the deprecated `...frequencyInSeconds` key and old fallback behavior.
Now that deprecated keys are removed, please update the documentation to match
the `...frequencyPeriod`-only lookup and default behavior.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -712,14 +664,13 @@ public int
getOfflineSegmentIntervalCheckerFrequencyInSeconds() {
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(ControllerPeriodicTasksConf.DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS);
Review Comment:
The Javadoc immediately above this method still references deprecated keys
and old fallback behavior (e.g., `...frequencyInSeconds`). After removing
deprecated config support, please update the documentation to describe the
`...frequencyPeriod`-only behavior and defaulting semantics.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -736,14 +687,13 @@ 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));
+ .map(period -> (int) convertPeriodToSeconds(period))
+
.orElse(ControllerPeriodicTasksConf.DEFAULT_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS);
Review Comment:
The Javadoc above this method still mentions the deprecated
`...frequencyInSeconds` config and old fallback behavior. Since this getter now
only reads `...frequencyPeriod` (else default), please update the Javadoc
accordingly.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -1340,14 +1272,8 @@ public int getPinotTaskQueueWarningThreshold() {
* REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS
*/
public long getSegmentRelocatorInitialDelayInSeconds() {
- Long segmentRelocatorInitialDelaySeconds =
-
getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS,
Long.class);
- if (segmentRelocatorInitialDelaySeconds == null) {
- segmentRelocatorInitialDelaySeconds =
-
getProperty(ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS,
- ControllerPeriodicTasksConf.getRandomInitialDelayInSeconds());
- }
- return segmentRelocatorInitialDelaySeconds;
+ return
getProperty(ControllerPeriodicTasksConf.SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS,
+ ControllerPeriodicTasksConf.getRandomInitialDelayInSeconds());
Review Comment:
The Javadoc for `getSegmentRelocatorInitialDelayInSeconds()` still says it
falls back to the deprecated realtime segment relocation initial delay key, but
that fallback was removed in this change. Please update the comment to match
the current behavior (new key + default random initial delay).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -876,25 +822,13 @@ 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);
- if (segmentRelocatorFreqSeconds == null) {
- String realtimeSegmentRelocatorPeriod =
-
getProperty(ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_RELOCATOR_FREQUENCY);
- if (realtimeSegmentRelocatorPeriod != null) {
- segmentRelocatorFreqSeconds = (int)
convertPeriodToSeconds(realtimeSegmentRelocatorPeriod);
- } else {
- segmentRelocatorFreqSeconds =
ControllerPeriodicTasksConf.DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS;
- }
- }
- return segmentRelocatorFreqSeconds;
- });
+ .map(period -> (int) convertPeriodToSeconds(period))
+
.orElse(ControllerPeriodicTasksConf.DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS);
Review Comment:
The Javadoc above `getSegmentRelocatorFrequencyInSeconds()` still references
deprecated keys (e.g., legacy realtime segment relocator config /
`...frequencyInSeconds`) and describes an order of preference that no longer
exists. Please update the Javadoc to reflect the current `...frequencyPeriod` +
default behavior.
##########
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:
The deprecated-config fallback tests were removed, but there is no longer a
test covering the behavior when a `*FREQUENCY_PERIOD` value is present but
invalid (e.g., verifying the getter returns the documented default and that the
invalid value is recorded in `getInvalidConfigs()`). Adding a small regression
test for invalid period specs would help ensure the post-deprecation behavior
remains stable.
--
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]
