Junping Du commented on YARN-2051:

Again, good work, [~decster]!
Some comments below, most of them are trivial:

+    System.out.printf("Validate %s %s\n", recordClass.getName(),
+        protoClass.getName());
Please replace this and other places that try to print to console with LOG.

+        ret = Sets.newHashSet(genTypeValue(params[0]));        
Please remove unnecessary space in the end of this line.

throw new IllegalArgumentException("type not support: " + type);
May be "type: " + type + " is not supported" is more readable?

+  private static Object genByNewInstance(Class clazz) throws Exception {
generateNewInstance() sounds like a better name?

ret = newInstance.invoke(null, args);
The code here has risk of NPE if newInstance method is not found previously (it 
is possible, as newInstance() method is not forced to have, although most class 
obey this rule). Better to add some exception handling here.

+      } else if (clazz.equals(ByteBuffer.class)) {
+        // return new ByteBuffer every time
+        // to prevent potential side effects
+        return ByteBuffer.allocate(4);
+      }
What's reasonable value we generate here for ByteBuffer? Just empty. Isn't it?

> Fix code bug and add more unit tests for PBImpls
> ------------------------------------------------
>                 Key: YARN-2051
>                 URL: https://issues.apache.org/jira/browse/YARN-2051
>             Project: Hadoop YARN
>          Issue Type: Test
>            Reporter: Junping Du
>            Assignee: Binglin Chang
>            Priority: Critical
>         Attachments: YARN-2051.v1.patch
> From YARN-2016, we can see some bug could exist in PB implementation of 
> protocol. The bad news is most of these PBImpl don't have any unit test to 
> verify the info is not lost or changed after serialization/deserialization. 
> We should add more tests for it.

This message was sent by Atlassian JIRA

Reply via email to