adoroszlai commented on code in PR #3672:
URL: https://github.com/apache/ozone/pull/3672#discussion_r953500522
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/ReplicationOptions.java:
##########
@@ -81,7 +81,21 @@ public void setReplication(String replication) {
}
// Option is defined in subclasses
- public void setType(ReplicationType type) {
- this.type = type;
+ public void setType(String type) {
+ try {
+ ReplicationType replicationType = ReplicationType.valueOf(type);
+ if (replicationType == ReplicationType.CHAINED
+ || replicationType == ReplicationType.STAND_ALONE) {
+ throw new IllegalArgumentException(
+ String.format("Unsupported replication type %s",
+ replicationType.name()));
+ }
+ this.type = replicationType;
+ } catch (IllegalArgumentException ex) {
+ throw new IllegalArgumentException(
+ String.format("Invalid value '%s' for option '--type'. %s." +
+ " Supported values are: RATIS, EC.",
+ type, ex.getMessage()));
+ }
Review Comment:
The error message for unsupported replication type is now more cumbersome
than previously:
```
Could not invoke public void
org.apache.hadoop.ozone.freon.FreonReplicationOptions.setType(java.lang.String)
with STAND_ALONE (java.lang.IllegalArgumentException: Invalid value 'CHAINED'
for option '--type'. Unsupported replication type CHAINED. Supported values
are: RATIS, EC.)
```
and
```
Could not invoke public void
org.apache.hadoop.ozone.shell.ShellReplicationOptions.setType(java.lang.String)
with asdf (java.lang.IllegalArgumentException: Invalid value 'asdf' for option
'--type'. No enum constant org.apache.hadoop.hdds.client.ReplicationType.asdf.
Supported values are: RATIS, EC.)
```
Ideally it should be:
```
Unsupported replication type CHAINED. Supported values are: RATIS, EC.
```
and
```
Unsupported replication type asdf. Supported values are: RATIS, EC.
```
So we should get rid of the message about invoking `setType`, and simplify
the Ozone-specific part (which is due to two levels of
`IllegalArgumentException`).
To achieve this, I think we may need a more complex change. So I guess this
one is OK as a temporary solution.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -1008,9 +1005,6 @@ public void testSetECReplicationConfigOnBucket() throws
Exception {
.equals(ECKeyOutputStream.class.getName()));
}
- args = new String[] {"bucket", "set-replication-config", bucketPath, "-t",
- "STAND_ALONE", "-r", "ONE"};
Review Comment:
This part of the test sets non-EC replication on the bucket which had EC
replication previously, then verifies the new key created is non-EC. Just
removing the "set-replication" call causes the key to be created with EC, and
the test to fail:
https://github.com/apache/ozone/runs/7987740557?check_suite_focus=true#step:5:3176
So we should keep the "set-replication" call and simply change
`STAND_ALONE/ONE` to `RATIS/THREE` 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
