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 2022/04/11 10:44:37 UTC
[GitHub] [ozone] adoroszlai opened a new pull request, #3293: HDDS-6565. EC: Unify replication-related CLI params
adoroszlai opened a new pull request, #3293:
URL: https://github.com/apache/ozone/pull/3293
## What changes were proposed in this pull request?
Various `ozone` commands have slightly different parameters for replication type, factor, and config (new for EC). The goal of this task:
* unify these params to provide more consistent user experience
* reduce code duplication
* keep compatibility with old parameters as far as possible
This is achieved by moving replication options into a separate class and use it as mixin. It provides methods to get settings from CLI parameters, from client-side config.
Replication options come in two flavors:
* shell: supports `-t`, but not legacy `--factor`
* freon: supports `--factor`, but not `-t`, because that is used for number of threads
Removed reference to `STAND_ALONE` and misspelled `STANDALONE` from option help.
https://issues.apache.org/jira/browse/HDDS-6565
## How was this patch tested?
Regular CI:
https://github.com/adoroszlai/hadoop-ozone/actions/runs/2141281821
--
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 a diff in pull request #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3293:
URL: https://github.com/apache/ozone/pull/3293#discussion_r848571151
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -141,9 +141,6 @@ public static void init() throws Exception {
testFile.getParentFile().mkdirs();
testFile.createNewFile();
Review Comment:
I've found option values are not cleared when `@Option` annotation is present on setter method instead of field. I need to set it on method to be able to override behavior in specific subclass `FreonReplicationOptions`/`ShellReplicationOptions`. So creating a new instance of `ozoneShell` for each test is necessary to avoid leaking the parameters. Otherwise test methods that do not set all replication params would fail.
BTW, this more closely reflects real-world usage of CLI commands (new instance for each invocation).
--
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] umamaheswararao merged pull request #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged PR #3293:
URL: https://github.com/apache/ozone/pull/3293
--
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] umamaheswararao commented on pull request #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on PR #3293:
URL: https://github.com/apache/ozone/pull/3293#issuecomment-1096819312
Thanks @adoroszlai for unifying the params. I got a question: Looks like most of the stuff can go into master directly?
How about general changing from factor to replicationconfig changes push into master first? Then we can handle ec changes here?
--
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] umamaheswararao commented on a diff in pull request #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3293:
URL: https://github.com/apache/ozone/pull/3293#discussion_r848508969
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -141,9 +141,6 @@ public static void init() throws Exception {
testFile.getParentFile().mkdirs();
testFile.createNewFile();
Review Comment:
is there a reason we want to initialize for every test now? ( init is annotated as @beforeclass. )
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientUtils.java:
##########
@@ -165,7 +165,7 @@ public static ReplicationConfig getClientConfiguredReplicationConfig(
*/
public static ReplicationConfig validateAndGetClientReplicationConfig(
ReplicationType userPassedType, String userPassedReplication,
- OzoneConfiguration clientSideConfig) {
Review Comment:
Can we update in resolveClientSideReplicationConfig also ?
--
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] umamaheswararao commented on a diff in pull request #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on code in PR #3293:
URL: https://github.com/apache/ozone/pull/3293#discussion_r848737647
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -141,9 +141,6 @@ public static void init() throws Exception {
testFile.getParentFile().mkdirs();
testFile.createNewFile();
Review Comment:
Thanks for the details. I agree on real-world case. I asked because to know why we are creating now and why it was not a problem before. I understand now. I am ok.
--
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 #3293: HDDS-6565. EC: Unify replication-related CLI params
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3293:
URL: https://github.com/apache/ozone/pull/3293#issuecomment-1097150542
Thank @umamaheswararao for reviewing and merging this.
--
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