[GitHub] [ozone] sodonnel commented on a change in pull request #2340: HDDS-5343 : EC: Add ECReplicationConfig into KeyInfo proto.

[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