Repository navigation
HDDS-16640. Make KeyLifecycleService configuration reconfigurable at runtime - #11721
Conversation
|
Thanks for the patch. To save CI resources, please consider creating PRs as draft, and keeping them that way until approved. (No need to change this now, just a note for future PRs.) |
These property names in the description not match the real name Will the "ozone admin reconfig --service=OM --address=om:9862 start" command display which property name is changed? @priyeshkaratha , could you add a section about reconfiguration in lifecycle.md and it's Chinese version? |
Please add documentation in ozone-site. Leftover docs are being removed from this repo in #10829. |
| public void reconfigure(String property, ConfigurationSource conf) { | ||
| switch (property) { | ||
| case OZONE_KEY_LIFECYCLE_SERVICE_ENABLED: | ||
| isServiceEnabled.set(conf.getBoolean(property, OZONE_KEY_LIFECYCLE_SERVICE_ENABLED_DEFAULT)); |
There was a problem hiding this comment.
I believe we should call the start() if the change is from false to enabled, and do nothing if it's from true-> false.
| /** | ||
| * Apply the new value of a property in {@link #OZONE_KEY_LIFECYCLE_PROPERTIES | ||
| * } without restarting OM. | ||
| * Changes to batch sizes and limits take effect on the next scheduled task. A new interval restarts the service, |
There was a problem hiding this comment.
"Changes to batch sizes and limits take effect on the next scheduled task" is not accurate, all the changes, except the interval, will take effect immediately on existing tasks' next batch scan.
| * @param property the property being reconfigured | ||
| * @param conf configuration holding the new value, or not holding the property to fall back to the default | ||
| */ | ||
| public void reconfigure(String property, ConfigurationSource conf) { |
There was a problem hiding this comment.
Instead of for each property change to take effect individually, is it possible that we can handle all the property change in one function in the same time?
There was a problem hiding this comment.
Yes, it is handled
| getSubject().reconfigureProperty(OZONE_KEY_LIFECYCLE_SERVICE_ENABLED, String.valueOf(originalEnabled)); | ||
| getSubject().reconfigureProperty(OZONE_KEY_LIFECYCLE_SERVICE_INTERVAL, originalInterval); | ||
| getSubject().reconfigureProperty(OZONE_KEY_LIFECYCLE_SERVICE_WORKERS, originalWorkers); | ||
| assertEquals(originalEnabled, service.status().getIsEnabled()); |
There was a problem hiding this comment.
Could you also add test for unset case?
|
Hi @ChenSammi Addressed review comments in latest commit. Created https://issues.apache.org/jira/browse/HDDS-17499 for document changes. |
|
Thanks @priyeshkaratha for updating the patch. One minor comment is we'd better change the log message from "KeyLifecycleService is suspended, disabled, or leader not ready" to ""KeyLifecycleService is suspended, disabled, leader not ready, or about to restart". And the introduced "OZONE_KEY_LIFECYCLE_PROPERTIES" reminds me that maybe we should extract all the key lifecycle properties from OMConfigKeys, to a new file, similar to "OmConfig", and extend the ReconfigurableConfig, that will make the logic very clear, and also easy to maintain. What do you think? |
Thanks @ChenSammi. It makes sense to me too. It may require more changes. I have created followup Jira HDDS-17515 for the same |
|
Thanks @ChenSammi for the review |
What changes were proposed in this pull request?
Before this change, changing any
ozone.om.key.lifecycle.service.*property meant restarting OM. This PR makes those properties reconfigurable at runtime through the OM reconfiguration framework (ozone admin reconfig --service=OM ... start).Reconfigurable properties (the new
KeyLifecycleService.OZONE_KEY_LIFECYCLE_PROPERTIESset):ozone.lifecycle.service.enabledozone.lifecycle.service.intervalozone.lifecycle.service.timeoutozone.lifecycle.service.workersozone.lifecycle.service.delete.batch-sizeozone.lifecycle.service.mpu.abort.limit.per.taskozone.lifecycle.service.delete.cached.directory.max-counozone.lifecycle.service.state.save.interval.msozone.lifecycle.service.state.save.keys.processedozone.lifecycle.service.move.to.trash.enabledWhat is the link to the Apache JIRA
HDDS-16640
How was this patch tested?
Added test case and tested manually