[7156] Human Readable Controller Configs #7173
Conversation
1. Created period variables. 2. Modified getters of some 'inSeconds' variables to read the period config and fallback to the 'inSeconds' config is the former is missing.
…vertPeriodToSeconds
…perty(name) to get properties of type strings
|
No setters have been added for the newly added configs as configs are meant to be immutable and these methods are already deprecated for existing configs. Also, I cannot add labels for some reason. This PR needs the "release notes" label. |
| private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds"; | ||
| private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod"; |
There was a problem hiding this comment.
This one should not be changed. We don't expect people to configure this value in minutes or hours.
There was a problem hiding this comment.
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing of to seconds). Will address this.
| private static final String SEGMENT_COMMIT_TIMEOUT_SECONDS = "controller.realtime.segment.commit.timeoutSeconds"; | ||
| private static final String SEGMENT_COMMIT_TIMEOUT_PERIOD = "controller.realtime.segment.commit.timeoutPeriod"; |
There was a problem hiding this comment.
This one should not be changed
| * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit | ||
| * config does not override invalid period config) | ||
| */ | ||
| @Test(expectedExceptions = IllegalArgumentException.class) |
There was a problem hiding this comment.
Expected exceptions are good, but I prefer fielding specific exceptions at the specific points where we expect them. That makes it easy to add more code to the test, or re-factor things, etc. and not worry about the test passing if we accidentally get IllegalArgumentException some place else.
There was a problem hiding this comment.
That's a valid point @mcvsubbu. I just went with the simplest approach as the test was very trivial, and you pretty much can point where exactly the exception was thrown. Will change it.
| @@ -341,8 +406,9 @@ public String getDataDir() { | |||
| } | |||
|
|
|||
| public int getSegmentCommitTimeoutSeconds() { | |||
There was a problem hiding this comment.
I feel that these method names also need to be changed, or have a new method here and mark the old one as deprecated.
The behavior should be that the new one should be used if both are specified or only the new one be specified. If not, use the old one.
There was a problem hiding this comment.
"The behavior should be that the new one should be used if both are specified or only the new one be specified" - Isn't it what it is doing @jackjlli? Basically if the new config is absent, it falls-back to the older one. But method still does what it says - getSegmentCommitTimeoutSeconds.
There was a problem hiding this comment.
Right, I wrote this one because I saw some of the java docs for the methods have been adjusted but it talks about the other way around. So mention it here.
| * If it doesn't exist, returns the segment level validation interval. 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 config value for controller.offline.segment.interval.checker.frequencyInSeconds if |
There was a problem hiding this comment.
We should check the new one first and then the old one, right?
There was a problem hiding this comment.
Missed this, will update the docs.
|
Thanks for the review everyone, I will incorporate the suggested changes. |
1. Rolled-back changes to controller.realtime.segment.commit.timeoutPeriod and server.request.timeoutPeriod. 2. Corrected documentation of getters.
|
Addressed earlier comments |
Codecov Report
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
+ Coverage 73.49% 73.55% +0.06%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73826 +68
Branches 10637 10650 +13
============================================
+ Hits 54209 54306 +97
+ Misses 16005 15973 -32
- Partials 3544 3547 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Please make sure that you change only those that actually need to be in minutes or hours. If something needs to be in seconds (or even lower) chances are that the default values will work, and hardly anyone will change it. So, ew don't need to add extra code to introduce new config and deprecate old one, etc. Not to mention someone setting a very high value and maybe causing other things to break
| @Deprecated | ||
| // The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings | ||
| public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS = | ||
| "controller.validation.frequencyInSeconds"; | ||
| @Deprecated |
There was a problem hiding this comment.
Another (separate) task can be to remove the DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS config and related code. I believe this has been deprecated for a few releases now, we can remove it.
There was a problem hiding this comment.
All deprecated configs have been prefixed with DEPRECATED. I will create a new ticket to do this in other places.
There was a problem hiding this comment.
I was suggesting to just remove DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS. It has been there since 0.5.0. I understand this can mean some more code to remove. If that is too much, then you can choose to add a TODO there, saying that this config has been deprecated as of 0.5.0, so must be removed. Thanks
There was a problem hiding this comment.
Can you either remove DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS or add a comment before line 80 saying it has been deprecated since 0.5.0
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Show resolved
Hide resolved
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
|
Alsobe sure to document these in https://docs.pinot.apache.org/configuration-reference/controller I guess the convention is to keep the old config and mark them as deprecated in the doc, and add the new config. Be sure to add an example in the "Description" column. |
Yes @mcvsubbu actually I was my thought process along the lines of keeping the configuration consistent - That is, even if we always expect a config in seconds, and the user say gives 2m as the config, we convert it to 120s and use it. But your points show this can be fragile. I will remove such configs. |
This is a WIP. I was actually confident that some of these new configurations would be rolled-back, so I waited till a review was done. |
| // frequency configs | ||
| @Deprecated | ||
| public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds"; |
There was a problem hiding this comment.
I like the idea of adding DEPRECATED_ prefix to the ones that are about to deprecated in the next release. Maybe we should do that in this PR as well.
1. Prefixed DEPRECATED configs with DEPRECATED_ 2. Handled disabling using -1 for controller.task.frequencyPeriod. 3. Un-did changes to controller.statuschecker.waitForPushTimePeriod. 4. Enhanced unit tests.
There was a problem hiding this comment.
also, in all the changes that you make, if you can prefix the configs with a comment like this:
// Deprecated as of 0.8.0
Then, it is easy for someone to remove it a few releases down the road.
Other than these minor comments, LGTM
Thanks for your contribution
Hey sure @mcvsubbu it is a pleasure to contribute to Pinot :) I will make these changes and raise another PR after updating the documentation. Thanks for your thorough review. |
| @@ -152,8 +152,8 @@ protected void startController() | |||
| Map<String, Object> controllerConfig = getDefaultControllerConfiguration(); | |||
| // Perform realtime segment validation every second with 1 second initial delay. | |||
| controllerConfig | |||
| .put(ControllerConf.ControllerPeriodicTasksConf.REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1); | |||
| controllerConfig.put(ControllerConf.ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, 1); | |||
| .put(ControllerConf.ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1); | |||
There was a problem hiding this comment.
The deprecated configs should only be used in the getter/setter method for better cleanup purpose. Maybe we should start changing these config to the ones you introduced in this PR?
There was a problem hiding this comment.
Yes @jackjlli we need to stop using these deprecated configs in our tests. Can I raise another PR for this? I think they are used at too many places and changing it in this PR will add another round of review. Can raise it immediately after this one.
|
PR for docs update: pinot-contrib/pinot-docs#47 |
| @Deprecated | ||
| // The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings | ||
| public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS = | ||
| "controller.validation.frequencyInSeconds"; | ||
| @Deprecated |
There was a problem hiding this comment.
Can you either remove DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS or add a comment before line 80 saying it has been deprecated since 0.5.0
| int durationInSeconds = getRandomDurationInSeconds(); | ||
| //all deprecated configs should be valid | ||
| DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds)); | ||
| String randomPeriodInMinutes = getRandomPeriodInMinutes(); |
There was a problem hiding this comment.
In this case maybe it is ok, but in general, it is a good idea to set the seed for the random number up front, and print out the value of the seed (in system.out). This will make sure that if there is failure due to some random value, we can reproduce the test case by manually setting the seed to that value. Otherwise, we will face intermittent failures in tests without knowing why that happened.
There was a problem hiding this comment.
Ah this is a good learning. Will incorporate this change.
There was a problem hiding this comment.
While this definitely is needed for reproducing failing tests, in this case, simply printing the test configuration gives enough information to reproduce a failing test. So I have simply printing the config for any failing test.
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
|
Other than one minor request of adding a comment, we are good to merge this. |
1. Removed DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS. 2. Printed the configuration under test for failing assertions.
Removed the deprecated config |
mcvsubbu
added
release-notes
|
Marking this for release notes, as well as backward incompat (since one config has been removed. This config was deprecated in 0.5.0 version) |
Description
#7156
This PR deprecates certain controller configurations in favour of new configs that accept human-readable period strings.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
Does this PR fix a zero-downtime upgrade introduced earlier?
Does this PR otherwise need attention when creating release notes?
Release Notes
The following configurations have been updated. Only those configs have been updated for which we expect valid values to be in minutes or hours (that is, if we only expect a configuration to be supplied in seconds, such configurations haven't been changed). If both configs are present, the new one is picked. If both configs are present but the new config uses an incorrect representation of period string, an exception is thrown (it does not fallback to the old configuration).
controller.retention.frequencyInSecondscontroller.retention.frequencyPeriodcontroller.offline.segment.interval.checker.frequencyInSecondscontroller.offline.segment.interval.checker.frequencyPeriodcontroller.realtime.segment.validation.frequencyInSecondscontroller.realtime.segment.validation.frequencyPeriodcontroller.broker.resource.validation.frequencyInSecondscontroller.broker.resource.validation.frequencyPeriodcontroller.statuschecker.waitForPushTimeInSecondscontroller.statuschecker.waitForPushTimePeriodcontroller.statuschecker.frequencyInSecondscontroller.statuschecker.frequencyPeriodcontroller.task.frequencyInSecondscontroller.task.frequencyPeriodcontroller.minion.instances.cleanup.task.frequencyInSecondscontroller.minion.instances.cleanup.task.frequencyPeriodcontroller.minion.instances.cleanup.task.minOfflineTimeBeforeDeletionSecondscontroller.minion.instances.cleanup.task.minOfflineTimeBeforeDeletionPeriodcontroller.minion.task.metrics.emitter.frequencyInSecondscontroller.minion.task.metrics.emitter.frequencyPeriodcontroller.segment.relocator.frequencyInSecondscontroller.segment.relocator.frequencyPeriodcontroller.segment.level.validation.intervalInSecondscontroller.segment.level.validation.intervalPeriodSome examples of updated configurations:
Documentation
Final PR for docs update will be raised once this PR is approved.