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