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 2021/04/09 09:09:50 UTC
[GitHub] [ozone] elek opened a new pull request #2136: HDDS-5073. Use ReplicationConfig on client side
elek opened a new pull request #2136:
URL: https://github.com/apache/ozone/pull/2136
## What changes were proposed in this pull request?
HDDS-5011 introduced new ReplicationConfig with multiple implementations (Ratis/Standalone).
To make ozone sh client EC compatible we can use ReplicationConfig on the client side too: --replication parameter can be parsed from the string based on the ReplicationConfig. Today it can be THREE or ONE, but later ECReplicationConfig can support more sophisticated schemes (like 3:2)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5073
## How was this patch tested?
With local docker-compose cluster + CI
No logic has been changed only the internal representation of the data.
--
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.
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] elek commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-842109164
ping @mukul1987, @umamaheswararao
--
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.
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] symious commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
symious commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620243636
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
Thanks for the update. At first, I thought ReplicationFactor is only for legacy configs and won't be used for EC schemas.
--
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.
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] arp7 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r633674668
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
I don't like the idea of an open string based config either. ENUMs are not inherently dangerous. Strings will be far worse.
The bigger issue is a lack of discipline in thinking through compatibility concerns.
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r630083971
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
@mukul1987 Do you have any more questions or concerns?
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r633325151
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
I am not comfortable with the idea of an open string-based config. ENUMS are not dangerous every time when we transfer them over the network.
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r619846095
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/StorageInfo.java
##########
@@ -133,7 +133,8 @@ public void setClusterId(String clusterId) {
}
private void verifyNodeType(NodeType type)
- throws InconsistentStorageStateException {
+ throws
Review comment:
unintended changes ?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationFactor.java
##########
@@ -71,6 +71,8 @@ public static ReplicationFactor fromProto(
}
}
+
Review comment:
unintended changes ?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
There are chances that both of these calls will throw errors? Whats the expected type of the factorString ?
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
why, do we want to convert replication factor to a string ? What are the expected strings which we expect after EC implementation ?
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r625036380
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
No. For client to server communication we use protobuf structures where each of the fields (data/parity and fator/type) are serialized. String is used only to easily select the right `ReplicationConfiguration` instance.
Please also note that for `RatisReplicationConfig` and `StandaloneReplicationConfig` the string is converted to an enum.
For the client parameter we need a string as we don't know how the value is parsed. It depends on the `ReplicationConfig` implementation.
However, the string parsing in `ECReplicationConfig` can be more strict. We can fix which data/parity pairs are acceptable today (with either enum or some rules) I didn't do in this patch as it can make the testing harder in current phase. Today I can create pipeline where data=2 parity=1. It's not something good for production but for testing it requires only 3 nodes.
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r621414525
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
I feel keeping values as strings opens up the whole pandora box of validations to do.
Is there a list of terminal policies this config will translate to ? I mean with client to server communication, do we plan to use string to pass the code info ?
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620227187
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
Good question.
I think we can start with `d:p` pattern where `d` is the number of data parts and `p` is the number of the parity part. But we can also extend it later with codec. At first, we can support `3:2` and `6:4` and `10:3`
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r621074907
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
That's a good question (thanks for it), and I think there are multiple parts.
1. This is only for `ozone sh` commands not for s3 (where we have storage classes) or for o3fs/ofs (where we plan to configure it cluster wide or bucket level.
2. Originally I had the same idea. `ReplicationConfig` today can have multiple parameters. (For example `3:2` can be defined as `new ECReplicationConfig(3,2)`). I thought that it might be better to accept only enums (or pre-defined strings) which are further defined by the administrators (or hard-coded by the developers):
For example:
| replication type | enum / group name | replication config |
|----------------------------|---|---|
| RATIS | ONE | `new RatisReplicationConfig(ONE) |
| RATIS | THREE | `new RatisReplicationConfig(THREE) |
| STANDALONE | ONE | `new StandaloneReplicationConfig(ONE) |
| EC | `3:2` | `new EcReplicationConfig(3:2)` |
| EC | `6:4` | `new EcReplicationConfig(6:4)` |
| EC | `10:3` | `new EcReplicationConfig(10:3)` |
But when we add the `codec` to `ECReplicationConfig` (`RS` vs `XOR`) it will create more and more entries in this table.
1. I think (at least at long term) administrators should have the flexibility to define new groups (shouldn't be everything hardcoded in the code)
2. I think it's an additional complexity to manage all of these mappings.
As `ozone sh` is a low level cli tool, it seems to be easier to avoid this grouping and just enable to directly configure all possible parameters (especially as there is no limit on the SCM/OM side, we can handle different configuration without any problem)
3. Compatibility
I also felt that this approach is a more natural extension to the current scheme. We can introduce a new abstraction level (with new cli parameters like `--storage-type`) where we accept names instead of low level parameters:
| name | replication type | replication config |
|----------------------------|---|---|
| REDUCED | RATIS | `new RatisReplicationConfig(ONE) |
| DEFAULT | RATIS | | `new RatisReplicationConfig(THREE) |
| STANDALONE | `new StandaloneReplicationConfig(ONE) |
| EC_3_2 | EC | `new EcReplicationConfig(3:2)` |
But I am not sure if we need this abstraction level, especially at the beginning of the feature branch.
4. Enum vs string
When you asked about enum, we can talk about two parts: do we need pre-defined groups AND/OR do we need to store in enum the groups?
I learned with the port changes that enum is very dangerous. We introduced a new service on DN and added a new enum value for the service discovery API and introduced a lot of backward compatibility problem (new enum values couldn't be deserialized with old clients). String seems to be more flexible (we can validate it after protobuf conversion).
This example doesn't fully apply here, as it's a full client-only change. (on protobuf this string is not sent), but I learned that enums (in some cases) can be harder
TLDR: This is why I think the string based approach is better here, but these are somewhat subjective points, please add your opinion.
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r635094043
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
> ENUMS are not dangerous every time when we transfer them over the network.
Thanks the comment @mukul1987 and @arp7
As we discussed during the EC meeting it's NOT about protobuf changes. It's a client side only change.
In general: we need multiple type of replication configs. For example:
```
new RatisReplicationConfig(ONE)
new RatisReplicationConfig(THREE)
new StandaloneReplicationConfig(ONE)
new StandaloneReplicationConfig(THREE)
new ECReplicationConfig(2,1,XOR)
new ECReplicationConfig(4,3,RS)
new ECReplicationConfig(6,4,RS)
new ECReplicationConfig(10,1,RS)
```
All of these configs are strongly typed both in Java code (OM/SCM) and protobuf. ONE/THREE are enums as before.
The only problem here is that Enum types / replication definitions **depend on the replication config type**. EC may need different enum values. First we need to choose the replication config based on the type (EC/RATIS/STANDLAONE) and **after that** we can ask the selected `ReplicationConfig` to parse the string to `Enum`. This is what we do in this patch.
It's a separated discussion if we need to validate the input string in `ECReplicationConfig` based on predefined enums or based on smart validation rules. Both can work, and please add your opinion to https://issues.apache.org/jira/browse/HDDS-5247
> The bigger issue is a lack of discipline in thinking through compatibility concerns.
Thanks the feedback @arp7
Can you please share your compatibility concerns?
This is a client side change and as you can see none of the smoketests are changed, so we can assume it's fully compatible. `ozone.replication` is also changed, but it accepts both integer and string existing values work as before.
--
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.
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] symious commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
symious commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r619669228
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
Should we throw an exception explicitly 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.
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] elek merged pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek merged pull request #2136:
URL: https://github.com/apache/ozone/pull/2136
--
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.
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] arp7 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r633674668
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
I don't like the idea of an open string based config either. ENUMs are not inherently dangerous. Strings will be far worse.
The bigger issue is a lack of discipline in thinking through compatibility concerns.
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620363618
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
Why do we want this config to be a string ? can cant we expose this as known set of enums ? cc @umamaheswararao @arp7
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r633325151
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
I am not comfortable with the idea of an open string-based config. ENUMS are not dangerous every time when we transfer them over the network.
--
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.
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] mukul1987 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r619846095
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/StorageInfo.java
##########
@@ -133,7 +133,8 @@ public void setClusterId(String clusterId) {
}
private void verifyNodeType(NodeType type)
- throws InconsistentStorageStateException {
+ throws
Review comment:
unintended changes ?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationFactor.java
##########
@@ -71,6 +71,8 @@ public static ReplicationFactor fromProto(
}
}
+
Review comment:
unintended changes ?
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
There are chances that both of these calls will throw errors? Whats the expected type of the factorString ?
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
why, do we want to convert replication factor to a string ? What are the expected strings which we expect after EC implementation ?
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620235297
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
> Should we throw an exception explicitly here?
Currently, `IllegalArgumentException` is thrown with the message:
```
Exception in thread "main" java.lang.IllegalArgumentException: No enum constant org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.asd
```
I think the type (`IllegalArgumentException`) is fine, but I modified the error message with throwing explicit exception:
```
Exception in thread "main" java.lang.IllegalArgumentException: Invalid RatisReplicationFactor 'xxx'. Please use ONE or THREE
```
--
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.
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] symious commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
symious commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r619669228
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
Should we throw an exception explicitly 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.
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] elek commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-845728791
@mukul1987 @umamaheswararao @arp7: Can we merge it? Do you have any more concerns?
--
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.
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] symious commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
symious commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-826111275
I saw there are some classes and tests still using the legacy replicationType and replicationFactor, for example, ObjectEndpiont.put().
The source branch is named "client-only" and if the work of updating other classes hasn't started, maybe I can help with the work?
--
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.
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] arp7 commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
arp7 commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r638034571
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/CopyKeyHandler.java
##########
@@ -82,17 +84,19 @@ protected void execute(OzoneClient client, OzoneAddress address)
OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
OzoneBucket bucket = vol.getBucket(bucketName);
- if (replicationFactor == null) {
- replicationFactor = ReplicationFactor.valueOf(
- getConf().getInt(OZONE_REPLICATION, OZONE_REPLICATION_DEFAULT));
- }
-
if (replicationType == null) {
replicationType = ReplicationType.valueOf(
- getConf().get(OZONE_REPLICATION_TYPE,
- OZONE_REPLICATION_TYPE_DEFAULT));
+ getConf()
+ .get(OZONE_REPLICATION_TYPE, OZONE_REPLICATION_TYPE_DEFAULT));
}
+ if (replication == null) {
Review comment:
+1 from me also with client-only change (i.e. no strings on the wire)
--
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.
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] elek commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-842109164
ping @mukul1987, @umamaheswararao
--
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.
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] symious commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
symious commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-826111275
I saw there are some classes and tests still using the legacy replicationType and replicationFactor, for example, ObjectEndpiont.put().
The source branch is named "client-only" and if the work of updating other classes hasn't started, maybe I can help with the work?
--
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.
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] elek commented on pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#issuecomment-847787007
Thanks the review @sodonnel, @symious @mukul1987 @arp7
I am merging it now...
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620230245
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/StorageInfo.java
##########
@@ -133,7 +133,8 @@ public void setClusterId(String clusterId) {
}
private void verifyNodeType(NodeType type)
- throws InconsistentStorageStateException {
+ throws
Review comment:
yes, reverting it. Thanks for the notification.
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationFactor.java
##########
@@ -71,6 +71,8 @@ public static ReplicationFactor fromProto(
}
}
+
Review comment:
reverted, thanks.
--
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.
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] elek commented on a change in pull request #2136: HDDS-5073. Use ReplicationConfig on client side
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2136:
URL: https://github.com/apache/ozone/pull/2136#discussion_r620229602
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/RatisReplicationConfig.java
##########
@@ -35,6 +35,16 @@ public RatisReplicationConfig(ReplicationFactor replicationFactor) {
this.replicationFactor = replicationFactor;
}
+ public RatisReplicationConfig(String factorString) {
+ ReplicationFactor factor = null;
+ try {
+ factor = ReplicationFactor.valueOf(Integer.parseInt(factorString));
+ } catch (NumberFormatException ex) {
+ factor = ReplicationFactor.valueOf(factorString);
Review comment:
In general the expected values are `ONE` and `THREE` but this code adds support to parse `ozone.replication` which can be 1 and 3 today. Long-term it will enable to use `3:2` in the `ozone.replication` (but we also need to configure EC as default `replicationType`)
--
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.
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