You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/08/16 13:03:18 UTC
[GitHub] [ozone] cchenax opened a new pull request #2537: HDDS-5482 Scm and datanode dynamic configuration adjustment
cchenax opened a new pull request #2537:
URL: https://github.com/apache/ozone/pull/2537
## What changes were proposed in this pull request?
scm and datanode dynamic configuration adjustment
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5482
## How was this patch tested?
ci
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2537: HDDS-5482 Scm and datanode dynamic configuration adjustment
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2537:
URL: https://github.com/apache/ozone/pull/2537#issuecomment-1001530841
/pending questions brought up by @fapifta
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #2537: HDDS-5482 Scm and datanode dynamic configuration adjustment
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #2537:
URL: https://github.com/apache/ozone/pull/2537#issuecomment-947149483
Hello @cchenax,
sorry, I kind of forgot to get back to this PR, after I first saw that there are checkstyle issues, I thought I will review when CI gets green, and never got a notification later on, and unfortunately I just got back to this in my backlog.
All in all the initiative seems to be good and pretty much useful, but there are some overall things that I would like to discuss on this PR:
1. In the GenericCLI changes, there is code duplication, in ReconfigurationThread.run, and in getOldProperties.
2. Also I do not see the use of the ClassLoader instance we create in this duplicated code, and the two try-catch block we can merge into one.
3. In the HddsDataNodeService.reconfigurePropertyImpl method, there is a switch-case statement, which differentiates one property, while it does the same with it as in the default branch. Besides that the method at the end of the day always throws a ReconfigurationException, which we catch in the ReconfigurationThread and we print a stack trace, I don't think we need this.
4. I am unsure if we need, and why we need the change in hadoop-ozone/dev-support/intellij/.ozone-site.xml file, can you please shed some light on that?
5. As the --start option and startReconfiguration() method is added to GenericCLI, not just the SCM/OM/DN starter has this but other commands as well, wouldn't it be cleaner to add a ReconfigurableDeamonCommand interface, that can be implemented by SCM/OM/DN, perhaps later on by S3G and HTTPFS (if we merge it) starter commands, so that we do not need to have a new method in other parent commands, and with that the common logic could be in a GenericReconfigurableDaemonCommand class which can extend the GenericCLI, and then daemon starter commands can extend that to have the basics of the functionality?
Also the name of the command line option is not really implying that it will start a reconfiguration thread in the background, though with the suggested change in the class hierarchy might make this startup switch unnecessary, I am not convinced that the ability of reconfiguration should depend on a command line switch, if we have the ability we should provide it, is there a reason for a switch that enables/disables this functionality?
6. I am unsure why we need locking and synchronization in the startReconfigurationTask method, as we are coming from CLI, and this logic runs on one thread, one CLI command won't start two reconfig threads, or two daemons at once. Do I miss something when I think synchronization and locking is not necessary here?
7. Instead of having an infinite loop in the thread, that waits on an object and then times out waiting, so effectively sleeping, it would be nice to use a WatchService from java.nio, and only act if a config file is changing in the config directory. With that we would be able to watch for changes not just in ozone-site, but in other configuration files as well later on.
Nit things:
- There are a lot of changes that are just targeting formatting of old commited code, which makes it a bit hard to review, and if we want to fix those, I would love to see it in an other PR that aims for that. (e.g.: most of the changes in HddsDataNodeService, or the single change in GenerateOzoneRequiredConfigurations)
- hadoop-ozone/dev-support/intellij/.ozone-site.xml.swp is added accidentally to the PR
- checkstyle check should not fail, and CI should be green
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] github-actions[bot] closed pull request #2537: HDDS-5482 Scm and datanode dynamic configuration adjustment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #2537:
URL: https://github.com/apache/ozone/pull/2537
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] github-actions[bot] commented on pull request #2537: HDDS-5482 Scm and datanode dynamic configuration adjustment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2537:
URL: https://github.com/apache/ozone/pull/2537#issuecomment-1014966208
Thank you very much for the patch. I am closing this PR __temporarily__ as there was no activity recently and it is waiting for response from its author.
It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.
It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.
If you need ANY help to finish this PR, please [contact the community](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org