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/06/15 17:04:53 UTC
[GitHub] [ozone] umamaheswararao opened a new pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
umamaheswararao opened a new pull request #2340:
URL: https://github.com/apache/ozone/pull/2340
## What changes were proposed in this pull request?
Added EcReplicationConfig in KeyInfo and KeyArgs proto structures.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5343
## How was this patch tested?
Tested by writing with ECReplicationConfig based key
--
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] sodonnel commented on a change in pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2340:
URL: https://github.com/apache/ozone/pull/2340#discussion_r656399319
##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/TestOzoneClient.java
##########
@@ -171,4 +166,31 @@ public void testPutKeyRatisOneNode() throws IOException {
Assert.assertFalse(key.getModificationTime().isBefore(testStartTime));
}
}
+
+ @Test
+ public void testPutKeyWithECReplicationCopnfig() throws IOException {
Review comment:
Typo here - Conpnfig -> Config
--
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] sodonnel commented on a change in pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2340:
URL: https://github.com/apache/ozone/pull/2340#discussion_r656433998
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
##########
@@ -488,7 +488,8 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) {
.setCreationTime(keyInfo.getCreationTime())
.setModificationTime(keyInfo.getModificationTime())
.setReplicationConfig(ReplicationConfig
- .fromTypeAndFactor(keyInfo.getType(), keyInfo.getFactor()))
+ .fromProto(keyInfo.getType(), keyInfo.getFactor(),
+ keyInfo.getEcReplicationConfig()))
Review comment:
At the Java object side, we always have a generic ReplicationConfig, and the static method you mentioned will create the correct sort of object.
However at the proto level, we have:
* ReplicationType
* Replication Factor
* ECReplicationConfig
So when you get a proto message, you must use:
```
fromProto(
HddsProtos.ReplicationType type,
HddsProtos.ReplicationFactor factor,
HddsProtos.ECReplicationConfig ecConfig)
```
Which you patch does. However if you have a KeyInfo object, and you want to turn it to proto, you have to populate:
1. Type and Factor for non EC
2. Type and ECReplicationConfig for EC.
At the moment the builder is like this:
```
public KeyInfo getProtobuf(boolean ignorePipeline, int clientVersion) {
long latestVersion = keyLocationVersions.size() == 0 ? -1 :
keyLocationVersions.get(keyLocationVersions.size() - 1).getVersion();
List<KeyLocationList> keyLocations = new ArrayList<>();
for (OmKeyLocationInfoGroup locationInfoGroup : keyLocationVersions) {
keyLocations.add(locationInfoGroup.getProtobuf(
ignorePipeline, clientVersion));
}
KeyInfo.Builder kb = KeyInfo.newBuilder()
.setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
.setDataSize(dataSize)
.setType(replicationConfig.getReplicationType())
.setFactor(ReplicationConfig.getLegacyFactor(replicationConfig))
.setLatestVersion(latestVersion)
.addAllKeyLocationList(keyLocations)
.setCreationTime(creationTime)
.setModificationTime(modificationTime)
.addAllMetadata(KeyValueUtil.toProtobuf(metadata))
.addAllAcls(OzoneAclUtil.toProtobuf(acls))
.setObjectID(objectID)
.setUpdateID(updateID);
if (encInfo != null) {
kb.setFileEncryptionInfo(OMPBHelper.convert(encInfo));
}
return kb.build();
}
```
It never sets ECReplicationConfig anywhere. So it needs changed to this:
```
public KeyInfo getProtobuf(boolean ignorePipeline, int clientVersion) {
long latestVersion = keyLocationVersions.size() == 0 ? -1 :
keyLocationVersions.get(keyLocationVersions.size() - 1).getVersion();
List<KeyLocationList> keyLocations = new ArrayList<>();
for (OmKeyLocationInfoGroup locationInfoGroup : keyLocationVersions) {
keyLocations.add(locationInfoGroup.getProtobuf(
ignorePipeline, clientVersion));
}
KeyInfo.Builder kb = KeyInfo.newBuilder()
.setVolumeName(volumeName)
.setBucketName(bucketName)
.setKeyName(keyName)
.setDataSize(dataSize)
.setType(replicationConfig.getReplicationType());
if (replicationConfig instanceof ECReplicationConfig) {
kb.setEcReplicationConfig(((ECReplicationConfig) replicationConfig)
.toProto());
} else {
kb.setReplicationFactor(
ReplicationConfig.getLegacyFactor(replicationConfig));
}
kb.setFactor(ReplicationConfig.getLegacyFactor(replicationConfig))
.setLatestVersion(latestVersion)
.addAllKeyLocationList(keyLocations)
.setCreationTime(creationTime)
.setModificationTime(modificationTime)
.addAllMetadata(KeyValueUtil.toProtobuf(metadata))
.addAllAcls(OzoneAclUtil.toProtobuf(acls))
.setObjectID(objectID)
.setUpdateID(updateID);
if (encInfo != null) {
kb.setFileEncryptionInfo(OMPBHelper.convert(encInfo));
}
return kb.build();
}
```
--
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] umamaheswararao merged pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged pull request #2340:
URL: https://github.com/apache/ozone/pull/2340
--
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] sodonnel commented on pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #2340:
URL: https://github.com/apache/ozone/pull/2340#issuecomment-867000514
This change LGTM now, but it incorporates a change which we just committed to master. Therefore we should merge master to the EC branch, and then rebase this change so we can review just the smaller change again prior to committing it.
--
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] umamaheswararao commented on a change in pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2340:
URL: https://github.com/apache/ozone/pull/2340#discussion_r656419692
##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/TestOzoneClient.java
##########
@@ -171,4 +166,31 @@ public void testPutKeyRatisOneNode() throws IOException {
Assert.assertFalse(key.getModificationTime().isBefore(testStartTime));
}
}
+
+ @Test
+ public void testPutKeyWithECReplicationCopnfig() throws IOException {
Review comment:
Thank you. I will correct it.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
##########
@@ -488,7 +488,8 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) {
.setCreationTime(keyInfo.getCreationTime())
.setModificationTime(keyInfo.getModificationTime())
.setReplicationConfig(ReplicationConfig
- .fromTypeAndFactor(keyInfo.getType(), keyInfo.getFactor()))
+ .fromProto(keyInfo.getType(), keyInfo.getFactor(),
+ keyInfo.getEcReplicationConfig()))
Review comment:
Currently OmKeyInfo has generic type replicationConfig field.
fromProto method handled to create ECReplicationConfig object inside.
```
static ReplicationConfig fromProto(
HddsProtos.ReplicationType type,
HddsProtos.ReplicationFactor factor,
HddsProtos.ECReplicationConfig ecConfig) {
switch (type) {
case EC:
return new ECReplicationConfig(ecConfig);
case RATIS:
case STAND_ALONE:
return fromTypeAndFactor(type, factor);
default:
throw new UnsupportedOperationException(
"Not supported replication: " + type);
}
}
```
Similar to your suggestion handled in below protos where we have ECReplicationConfig defined.
I am not sure I fully get it, but when you want to convert to proto, isn't it like the low method(instance check) we use and set ECReplicationConfig?
Sure I will check tests to add.
--
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] sodonnel commented on a change in pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2340:
URL: https://github.com/apache/ozone/pull/2340#discussion_r656395261
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
##########
@@ -488,7 +488,8 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) {
.setCreationTime(keyInfo.getCreationTime())
.setModificationTime(keyInfo.getModificationTime())
.setReplicationConfig(ReplicationConfig
- .fromTypeAndFactor(keyInfo.getType(), keyInfo.getFactor()))
+ .fromProto(keyInfo.getType(), keyInfo.getFactor(),
+ keyInfo.getEcReplicationConfig()))
Review comment:
I think you need something like this (taken from ContainerInfo, which I changed in an earlier PR) in the getProtobuf(...) method of the same class. As it stands now, you will be able to inflate a Java object from the proto, but if you go from the Java object to Proto you will lose the ECReplicationConfig:
```
if (replicationConfig instanceof ECReplicationConfig) {
builder.setEcReplicationConfig(((ECReplicationConfig) replicationConfig)
.toProto());
} else {
builder.setReplicationFactor(
ReplicationConfig.getLegacyFactor(replicationConfig));
}
```
It would also be good to add a simple unit tests to validate the proto serialization and deserialization to ensure we have the logic correct for EC / Non EC replication config. Something like the one in TestContainerInfo
--
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