-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-4491. Deprecate raft.server.rpcslowness.timeout config key #1624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wycccccc for working on this. Unfortunately the patch is not enough to handle deprecated configs.
rpc.slowness.timeoutis only part of the config key name. It is defined inand should be used together with its prefix, which comes fromozone/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/DatanodeRatisServerConfig.java
Line 91 in a4cd12c
@Config(key = "rpc.slowness.timeout", ozone/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/DatanodeRatisServerConfig.java
Lines 34 to 35 in a4cd12c
@ConfigGroup(prefix = HDDS_DATANODE_RATIS_PREFIX_KEY + "." + RaftServerConfigKeys.PREFIX) - The
ConfigurationProviderchanged in this patch is for Recon, but the config key belongs to Datanode, another component of Ozone. TheDeprecationDeltashould be added somewhere inorg.apache.hadoop.hdds.confinhadoop-hdds. (Sidenote: I thinkDatanodeRatis*Configclasses belong tohadoop-hdds/container-service, nothadoop-hdds/framework.)
|
Thank @adoroszlai for your suggestion, I will try to modify it correctly. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wycccccc for updating the patch. The change looks good to achieve the desired goal. I think cohesion might be improved by adding the deprecation in RaftServerConfigKeys instead of OzoneConfiguration, but it may have downsides, too, so I'm OK with the current version.
Can you please fix checkstyle violations?
|
I have fixed checkstyle violations.Thanks @adoroszlai for your suggestion, it helped me a lot. |
|
Thanks @wycccccc for the contribution. |
What changes were proposed in this pull request?
Add old config key
rpcslowness.timeoutto handle deprecated configsWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4491
How was this patch tested?
Added unit test.