HDDS-3556 Refactor conf in SCMRatisServer to Java-based conf. #907
Conversation
| //TODO enable Ratis group | ||
| scmRatisServer = SCMRatisServer.newSCMRatisServer(configuration | ||
| .getObject(SCMRatisServer.SCMRatisServerConfiguration.class), | ||
| this, scmNodeDetails, Collections.EMPTY_LIST, |
There was a problem hiding this comment.
Passing half created object (this) to SCMRatisServer constructor could cause subtle bugs that are tricky to get right. We should avoid it if possible.
There was a problem hiding this comment.
I have made updates to have SCM use static method for createSCM and make constructor private. SCMRatisServer will be constructed after SCM is constructed.
| /** | ||
| * Configuration used by SCM Ratis Server. | ||
| */ | ||
| @ConfigGroup(prefix = "ozone.scm.ratis") |
There was a problem hiding this comment.
Is it possible to make these RATIS related configuration reusable among DN,SCM and OM?
Just different prefix like, ozone.om.ratis, ozone.scm.ratis, ozone.dn.ratis as prefix?
There was a problem hiding this comment.
Yea it looks like we currently have ozone.om.ratis, ozone.scm.ratis, hdds.datanode.ratis, dfs.container.ratis and even dfs.ratis.
If we decide to refactor all config into Java-based configuration, we shall consider refactoring all config to be like ozone.ratis.*
Then we can take a look at reusable configuration among OM, SCM and container. I create Jira to track: https://issues.apache.org/jira/browse/HDDS-3577
|
+1, LGTM. |
What changes were proposed in this pull request?
Refactor SCMRatisServer conf to java based.
(Please fill in changes proposed in this fix)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3556
(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
UT
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)