Skip to content

Conversation

@sarvekshayr
Copy link
Contributor

What changes were proposed in this pull request?

Renaming config keys and its corresponding variable name prefixed with 'dfs' to 'ozone'.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-815

How was this patch tested?

Patch was tested by running the test classes of the modified files.

Copy link
Contributor

@tanvipenumudy tanvipenumudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sarvekshayr for the patch.
Could you please rebase your patch on top of the latest master?

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sarvekshayr for working on this. When changing config keys, we have to consider backwards compatibility:

Existing clusters may already be configured using the old keys. If we simply change the keys, those configs will no longer work after upgrade. Cluster will suddenly start to use the default settings.

The way to keep old settings work is by deprecating the old key, which is usually done here:

private static void addDeprecatedKeys() {
Configuration.addDeprecations(new DeprecationDelta[]{
new DeprecationDelta("ozone.datanode.pipeline.limit",
ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT),
new DeprecationDelta(HDDS_DATANODE_RATIS_PREFIX_KEY + "."
+ RaftServerConfigKeys.PREFIX + "." + "rpcslowness.timeout",
HDDS_DATANODE_RATIS_PREFIX_KEY + "."
+ RaftServerConfigKeys.PREFIX + "." + "rpc.slowness.timeout"),
new DeprecationDelta("dfs.datanode.keytab.file",
DFSConfigKeysLegacy.DFS_DATANODE_KERBEROS_KEYTAB_FILE_KEY),
new DeprecationDelta("ozone.scm.chunk.layout",
ScmConfigKeys.OZONE_SCM_CONTAINER_LAYOUT_KEY),
new DeprecationDelta("hdds.datanode.replication.work.dir",
OZONE_CONTAINER_COPY_WORKDIR)
});
}

Also, please wait for clean CI run in your fork before opening PRs.

@sarvekshayr sarvekshayr marked this pull request as draft February 7, 2024 14:04
@sarvekshayr sarvekshayr marked this pull request as ready for review February 8, 2024 09:36
@adoroszlai
Copy link
Contributor

Thanks @sarvekshayr for updating the patch.

There are a few config keys with dfs. prefix left. Probably the list in the Jira issue is outdated.

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java
54:      = "dfs.container.ratis.num.write.chunk.threads.per.volume";
82:      "dfs.container.ratis.statemachinedata.sync.retries";
85:      "dfs.container.ratis.statemachine.max.pending.apply-transactions";
92:      "dfs.container.ratis.log.queue.num-elements";
96:      "dfs.container.ratis.log.queue.byte-limit";
101:      "dfs.container.ratis.log.appender.queue.num-elements";
105:      "dfs.container.ratis.log.appender.queue.byte-limit";
109:      "dfs.container.ratis.log.purge.gap";
114:      "dfs.container.ratis.leader.pending.bytes.limit";
119:      "dfs.ratis.server.retry-cache.timeout.duration";

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
68:      "dfs.container.ratis.datastream.random.port";
74:      "dfs.container.chunk.write.sync";
86:      "dfs.container.ratis.admin.port";
92:      "dfs.container.ratis.server.port";
99:      = "dfs.container.ratis.datastream.enabled";
103:      = "dfs.container.ratis.datastream.port";

Also, Java constants for default values of the renamed config keys still use the old DFS_ prefix. It would be nice to match names for key and default value. Example:

public static final String OZONE_CONTAINER_RATIS_IPC_PORT =
"ozone.container.ratis.ipc";
public static final int DFS_CONTAINER_RATIS_IPC_PORT_DEFAULT = 9858;

@sarvekshayr
Copy link
Contributor Author

sarvekshayr commented Feb 8, 2024

Thank you for the review @adoroszlai ! I'll rename the remaining ones as well. I'll create a sub-task to do the same in order to reduce the size of this patch.

@adoroszlai
Copy link
Contributor

To clarify my comment about separating changes: it's not just about the size of the patch itself.

Renaming config keys is a relatively small change, focused on a few files, but it must be well tested to ensure compatibility.

On the other hand, renaming constants is a massive, but simple change, affecting ~50 files in this case. Yet, regular CI checks are enough as a test; it should be fine as long as code compiles and checkstyle is happy. This part of the change is likely to cause merge conflicts for other PRs.

@sarvekshayr
Copy link
Contributor Author

Thank you for the clarification. I'll make the changes as indicated.

@adoroszlai
Copy link
Contributor

Updated HDDS-815 to reflect the current set of config keys to be renamed (6 keys no longer exist, 15 new keys).

The original task description mentioned using "either HDDS or Ozone" as prefix. I have updated it to use hdds. based on:

  • these keys belong to Datanode, which is a HDDS service (hadoop-hdds/container-service)
  • more existing keys are named hdds.container... than ozone.container...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants