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

[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