Copilot commented on code in PR #18001:
URL: https://github.com/apache/pinot/pull/18001#discussion_r3003367969
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -761,27 +715,25 @@ public void
setRealtimeOffsetAutoResetBackfillFrequencyInSeconds(int offsetAutoR
}
Review Comment:
`setRealtimeOffsetAutoResetBackfillFrequencyInSeconds()` writes a plain
integer string into `REALTIME_OFFSET_AUTO_RESET_BACKFILL_FREQUENCY_PERIOD`, but
the corresponding getter now treats this property as a period string (e.g.
"60s") and validates/parses it. As a result, values written by this setter will
be considered invalid and the getter will fall back to the default. Update the
setter to write a valid period string (e.g. append "s").
##########
pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java:
##########
@@ -292,6 +198,16 @@ private String getRandomString() {
return RandomStringUtils.randomAlphanumeric(5);
}
Review Comment:
After removing the deprecated-config tests, `getRandomString()` is no longer
referenced in this test class. Consider deleting it to avoid dead code in tests.
##########
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:
`getRealtimeConsumerMonitorRunFrequency()` falls back to
`ControllerPeriodicTasksConf.DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_PERIOD`, but
that constant is not defined in `ControllerPeriodicTasksConf` (only
`DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_IN_SECONDS` exists). This will not
compile. Define the missing period default (e.g., "-1s") or revert the fallback
to use the existing seconds default.
```suggestion
.orElse(ControllerPeriodicTasksConf.DEFAULT_RT_CONSUMER_MONITOR_FREQUENCY_IN_SECONDS);
```
--
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]
