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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
