Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: io
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is a demand for Hadoop-5438.
      Also another small improvement is that i saw that in the beginning of readObject, it tries to get the class from PRIMITIVE_NAMES and then conf. Maybe it is better to add a direct load after them if the delaredClass is still null. Like this:

      String className = UTF8.readString(in);
          Class<?> declaredClass = PRIMITIVE_NAMES.get(className);
          if (declaredClass == null) {
            try {
              declaredClass = conf.getClassByName(className);
            } catch (Exception e) {
            }
          }
          
          if(declaredClass == null) {
            try {
              declaredClass = Class.forName(className);
            } catch (ClassNotFoundException e) {
              throw new RuntimeException("readObject can't find class " + className, e);
            }
          }
      
      1. Hadoop-5596-2009-04-30-3.patch
        12 kB
        He Yongqiang
      2. Hadoop-5596-2009-04-30-1.patch
        11 kB
        He Yongqiang
      3. Hadoop-5596-2009-04-30.patch
        22 kB
        He Yongqiang
      4. Hadoop-5596-2009-04-24.patch
        7 kB
        He Yongqiang
      5. Hadoop-5596-2009-04-15-2.patch
        7 kB
        He Yongqiang
      6. Hadoop-5596-2009-04-15.patch
        7 kB
        He Yongqiang
      7. Hadoop-5596-2009-04-03.patch
        2 kB
        He Yongqiang
      8. Hadoop-5596-2009-03-31.patch
        2 kB
        He Yongqiang

        Issue Links

          Activity

          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #827 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/827/)
          . Add EnumSetWritable. Contributed by He Yongqiang

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #827 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/827/ ) . Add EnumSetWritable. Contributed by He Yongqiang
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.21.0 [ 12313563 ]
          Resolution Fixed [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Yongqiang!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Yongqiang!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12406845/Hadoop-5596-2009-04-30-3.patch
          against trunk revision 771231.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12406845/Hadoop-5596-2009-04-30-3.patch against trunk revision 771231. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/287/console This message is automatically generated.
          dhruba borthakur made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          dhruba borthakur added a comment -

          re-submit to trigger hudson tests.

          Show
          dhruba borthakur added a comment - re-submit to trigger hudson tests.
          dhruba borthakur made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          dhruba borthakur added a comment -

          RE-submit to trigger Hudson tests.

          Show
          dhruba borthakur added a comment - RE-submit to trigger Hudson tests.
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hadoop Flags [Reviewed]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 Hadoop-5596-2009-04-30-3.patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 Hadoop-5596-2009-04-30-3.patch looks good.
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-30-3.patch [ 12406845 ]
          Hide
          He Yongqiang added a comment -

          Thanks for the detailed instructions, Nicholas.
          Hadoop-5596-2009-04-30-3.patch finished

          Show
          He Yongqiang added a comment - Thanks for the detailed instructions, Nicholas. Hadoop-5596-2009-04-30-3.patch finished
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hadoop-5596-2009-04-30-1.patch is much better. Thanks, Yongqiang.

          • change
            +    EnumSetWritable<E> other = (EnumSetWritable<E>) o;
            

            to

            +    EnumSetWritable<?> other = (EnumSetWritable<?>) o;
            

            Then, the @SuppressWarnings above equals(..) can be removed.

          • I think we need @SuppressWarnings in above newInstance().
          • Public methods need javadoc.
          Show
          Tsz Wo Nicholas Sze added a comment - Hadoop-5596-2009-04-30-1.patch is much better. Thanks, Yongqiang. change + EnumSetWritable<E> other = (EnumSetWritable<E>) o; to + EnumSetWritable<?> other = (EnumSetWritable<?>) o; Then, the @SuppressWarnings above equals(..) can be removed. I think we need @SuppressWarnings in above newInstance(). Public methods need javadoc.
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-30-1.patch [ 12406835 ]
          Hide
          He Yongqiang added a comment -

          Hadoop-5596-2009-04-30.patch did some code format to ObjectWritable. Hadoop-5596-2009-04-30-1.patch cancles those extra format.

          Show
          He Yongqiang added a comment - Hadoop-5596-2009-04-30.patch did some code format to ObjectWritable. Hadoop-5596-2009-04-30-1.patch cancles those extra format.
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-30.patch [ 12406833 ]
          Hide
          He Yongqiang added a comment -

          Thanks, Nicholas.
          >>enforce the status that either both elementType and value are null, or both are not null.
          We now enforce if value is empty or null, elementType must provided.
          >>check null before accessing value in equals(..), hashCode(), toString(), etc. Otherwise, there are NPE.
          done.
          >>check null for conf.
          did it in ObjectWritable's loadClass.
          >>usage of @SuppressWarnings("unchecked")
          All the Warnings now are caused by ObjectWritable.

          pls take a look at the new patch file, if ok, i will submit it for patch testing.

          Show
          He Yongqiang added a comment - Thanks, Nicholas. >>enforce the status that either both elementType and value are null, or both are not null. We now enforce if value is empty or null, elementType must provided. >>check null before accessing value in equals(..), hashCode(), toString(), etc. Otherwise, there are NPE. done. >>check null for conf. did it in ObjectWritable's loadClass. >>usage of @SuppressWarnings("unchecked") All the Warnings now are caused by ObjectWritable. pls take a look at the new patch file, if ok, i will submit it for patch testing.
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > We only force an EnumSet not be none when it is serialized.
          Since EnumSetWritable is in org.apache.hadoop.io package, we have to make sure it works for all kind of EnumSet, including empty set.

          > If it is not ok. how about this, we require the elementType when we serialize a non enumset? if the elementType is not provided, we throw an exception.
          This is one possible solution. Then, we should enforce the status that either both elementType and value are null, or both are not null.

          More comments on the patch:

          • check null before accessing value in equals(..), hashCode(), toString(), etc. Otherwise, there are NPE.
          • Similarly, check null for conf.
          • need more tests for EnumSetWritable. The current test does not use EnumSetWritable at all.
          • minimize the usage of @SuppressWarnings("unchecked"). We should fix the generic syntax but not suppressing the warnings.
          Show
          Tsz Wo Nicholas Sze added a comment - > We only force an EnumSet not be none when it is serialized. Since EnumSetWritable is in org.apache.hadoop.io package, we have to make sure it works for all kind of EnumSet, including empty set. > If it is not ok. how about this, we require the elementType when we serialize a non enumset? if the elementType is not provided, we throw an exception. This is one possible solution. Then, we should enforce the status that either both elementType and value are null, or both are not null. More comments on the patch: check null before accessing value in equals(..), hashCode(), toString(), etc. Otherwise, there are NPE. Similarly, check null for conf. need more tests for EnumSetWritable. The current test does not use EnumSetWritable at all. minimize the usage of @SuppressWarnings("unchecked"). We should fix the generic syntax but not suppressing the warnings.
          Hide
          He Yongqiang added a comment -

          >>I think it is not acceptable since we cannot force an EnumSet to be non-empty.
          We only force an EnumSet not be none when it is serialized.
          If it is not ok. how about this, we require the elementType when we serialize a non enumset? if the elementType is not provided, we throw an exception.

          Show
          He Yongqiang added a comment - >>I think it is not acceptable since we cannot force an EnumSet to be non-empty. We only force an EnumSet not be none when it is serialized. If it is not ok. how about this, we require the elementType when we serialize a non enumset? if the elementType is not provided, we throw an exception.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > Btw,the only problem now is that we can not serialize a none enumset, is that not acceptable?
          I think it is not acceptable since we cannot force an EnumSet to be non-empty.

          > even if we convert the EnumSet to an array and serialize it, we can not sovle the none enumset problem.
          Sorry that I was not clear. I mean declaring an Enum array in the client protocol for HADOOP-5438.

          Show
          Tsz Wo Nicholas Sze added a comment - > Btw,the only problem now is that we can not serialize a none enumset, is that not acceptable? I think it is not acceptable since we cannot force an EnumSet to be non-empty. > even if we convert the EnumSet to an array and serialize it, we can not sovle the none enumset problem. Sorry that I was not clear. I mean declaring an Enum array in the client protocol for HADOOP-5438 .
          Hide
          He Yongqiang added a comment -

          >> adding the type information to EnumSetWritable like the EnumSet implemention? like private Class<E> elementType;
          Yeah, it will solve the problem. But the other problem is that users may does not know the elementType before deserialized it.

          >>we don't really need EnumSetWritable. We may convert the EnumSet to an array and serialize the array in RPC
          That is what i initally does in ObjectWritable. The i moved the code to EnumSetWritable. It is more clean now since ObjectWritable is a core class.

          Btw,the only problem now is that we can not serialize a none enumset, is that not acceptable? even if we convert the EnumSet to an array and serialize it, we can not sovle the none enumset problem.

          Show
          He Yongqiang added a comment - >> adding the type information to EnumSetWritable like the EnumSet implemention? like private Class<E> elementType; Yeah, it will solve the problem. But the other problem is that users may does not know the elementType before deserialized it. >>we don't really need EnumSetWritable. We may convert the EnumSet to an array and serialize the array in RPC That is what i initally does in ObjectWritable. The i moved the code to EnumSetWritable. It is more clean now since ObjectWritable is a core class. Btw,the only problem now is that we can not serialize a none enumset, is that not acceptable? even if we convert the EnumSet to an array and serialize it, we can not sovle the none enumset problem.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > i suspose we need to throw an exception when serialize an empty EnumSet?

          How about adding the type information to EnumSetWritable like the EnumSet implemention?

              private Class<E> elementType;
          

          On a second thought, we don't really need EnumSetWritable. We may convert the EnumSet to an array and serialize the array in RPC. This approach may be simpler.

          Show
          Tsz Wo Nicholas Sze added a comment - > i suspose we need to throw an exception when serialize an empty EnumSet? How about adding the type information to EnumSetWritable like the EnumSet implemention? private Class <E> elementType; On a second thought, we don't really need EnumSetWritable. We may convert the EnumSet to an array and serialize the array in RPC. This approach may be simpler.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12406328/Hadoop-5596-2009-04-24.patch
          against trunk revision 768376.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12406328/Hadoop-5596-2009-04-24.patch against trunk revision 768376. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/249/console This message is automatically generated.
          He Yongqiang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-24.patch [ 12406328 ]
          Hide
          He Yongqiang added a comment -

          Hadoop-5596-2009-04-24.patch fixes the empty EnumSet problem. It will throw UnsupportedOperationException if tries to serialize an empty EnumSet. Thanks, Tsz Wo (Nicholas), SZE. And also it will not throw uncheck warn .

          Show
          He Yongqiang added a comment - Hadoop-5596-2009-04-24.patch fixes the empty EnumSet problem. It will throw UnsupportedOperationException if tries to serialize an empty EnumSet. Thanks, Tsz Wo (Nicholas), SZE. And also it will not throw uncheck warn .
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          He Yongqiang added a comment -

          After several tries on this. I realized that EnumSet<E> is a generic type and i can not find a way to get E's real class name.
          So it make things difficult for EnumSet.noneOf(Clsss). Because if we serialize the non EnumSet, we have to write out E's class name.

          i suspose we need to throw an exception when serialize an empty EnumSet?

          Show
          He Yongqiang added a comment - After several tries on this. I realized that EnumSet<E> is a generic type and i can not find a way to get E's real class name. So it make things difficult for EnumSet.noneOf(Clsss). Because if we serialize the non EnumSet, we have to write out E's class name. i suspose we need to throw an exception when serialize an empty EnumSet?
          Hide
          He Yongqiang added a comment -

          Yes, Thanks a lot! I did not notice that EnumSet can have none values. I will correct that soon.

          Show
          He Yongqiang added a comment - Yes, Thanks a lot! I did not notice that EnumSet can have none values. I will correct that soon.
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • Seems to me that read and write are not consistent if EnumSetWritable.value is an empty set. We may have the following problem:
          EnumSetWritable w1 = new EnumSetWritable(EnumSet.noneOf(MyEnum.class));
          w1.write(out);
          
          ...
          
          EnumSetWritable w2 = new EnumSetWritable();
          w2.readFields(in);
          
          // w2.value is null but w1.value is an empty set
          
          Show
          Tsz Wo Nicholas Sze added a comment - Seems to me that read and write are not consistent if EnumSetWritable.value is an empty set. We may have the following problem: EnumSetWritable w1 = new EnumSetWritable(EnumSet.noneOf(MyEnum.class)); w1.write(out); ... EnumSetWritable w2 = new EnumSetWritable(); w2.readFields(in); // w2.value is null but w1.value is an empty set Also, could you fix the unchecked warnings? The declaration properly should be EnumSetWritable<E extends Enum<E>>. See also http://java.sun.com/javase/6/docs/api/java/util/EnumSet.html
          Hide
          He Yongqiang added a comment -

          Requesting if somebody can review the patch and comment. I have another patch for h-5480 suspend there. Thanks a lot!

          Show
          He Yongqiang added a comment - Requesting if somebody can review the patch and comment. I have another patch for h-5480 suspend there. Thanks a lot!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12405515/Hadoop-5596-2009-04-15-2.patch
          against trunk revision 766670.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12405515/Hadoop-5596-2009-04-15-2.patch against trunk revision 766670. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/216/console This message is automatically generated.
          dhruba borthakur made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          dhruba borthakur added a comment -

          Trigger HadoopQA tests.

          Show
          dhruba borthakur added a comment - Trigger HadoopQA tests.
          dhruba borthakur made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          dhruba borthakur added a comment -

          Re-submit for HadoopQA tests on latest version of patch.

          Show
          dhruba borthakur added a comment - Re-submit for HadoopQA tests on latest version of patch.
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-15-2.patch [ 12405515 ]
          Hide
          He Yongqiang added a comment -

          fixes a compile warn.

          Show
          He Yongqiang added a comment - fixes a compile warn.
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-15.patch [ 12405510 ]
          Hide
          He Yongqiang added a comment -

          1) added in an EnumSetWritable, which is just a writable wrapper of EnumSet.
          2) made a small modification of ObjectWritable, in case when a null conf is passed in, it does not fail.
          3) a small testcase for EnumSetWritable
          thanks, Nicholas.

          BTW, pls close this issue or commit the code, so i can continue working on H-5438. If it is closed, i will merge this patch with H-5438.

          Show
          He Yongqiang added a comment - 1) added in an EnumSetWritable, which is just a writable wrapper of EnumSet. 2) made a small modification of ObjectWritable, in case when a null conf is passed in, it does not fail. 3) a small testcase for EnumSetWritable thanks, Nicholas. BTW, pls close this issue or commit the code, so i can continue working on H-5438. If it is closed, i will merge this patch with H-5438.
          Hide
          He Yongqiang added a comment -

          Thanks,Nicholas. It will be good if we do as you commented in H-5438:
          "We may declare FileSystem.create(..) with EnumSet and ClientProtocol.create(..) with EnumSetWritable. "
          In this way, EnumSetWritable can be used in other apps, just as LongWritable etc.

          Show
          He Yongqiang added a comment - Thanks,Nicholas. It will be good if we do as you commented in H-5438: "We may declare FileSystem.create(..) with EnumSet and ClientProtocol.create(..) with EnumSetWritable. " In this way, EnumSetWritable can be used in other apps, just as LongWritable etc.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > It would be more comfortable for users to use EnumSet directly.

          Why it would be more comfortable? In the FileSystem, there are many classes like FileStatus, BlockLocation, etc., implementing the Writable interface.

          Show
          Tsz Wo Nicholas Sze added a comment - > It would be more comfortable for users to use EnumSet directly. Why it would be more comfortable? In the FileSystem, there are many classes like FileStatus, BlockLocation, etc., implementing the Writable interface.
          Hide
          He Yongqiang added a comment -

          It would be more comfortable for users to use EnumSet directly. The create() is an interface which interacts directly with programmers. So it would be better if we keep it as simple as possible.

          Show
          He Yongqiang added a comment - It would be more comfortable for users to use EnumSet directly. The create() is an interface which interacts directly with programmers. So it would be better if we keep it as simple as possible.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          It should work if create(..) is declared with EnumSetWritable, instead of EnumSet.

          Show
          Tsz Wo Nicholas Sze added a comment - It should work if create(..) is declared with EnumSetWritable, instead of EnumSet.
          Hide
          He Yongqiang added a comment -

          Thanks,Nicholas.
          This issue is a demand of fixing hadoop-5438. In 5438, we added a new parameter of EnumSet type in namenode's create(). So the RPC part needs to serialize the EnumSet in its Invocation object, which directly call ObjectWritable.
          If we create a new EnumSetWritable here, can it be used by the RPC part without changing the type of our new added create parameter from EnumSet to EnumSetWritable?

          Show
          He Yongqiang added a comment - Thanks,Nicholas. This issue is a demand of fixing hadoop-5438. In 5438, we added a new parameter of EnumSet type in namenode's create(). So the RPC part needs to serialize the EnumSet in its Invocation object, which directly call ObjectWritable. If we create a new EnumSetWritable here, can it be used by the RPC part without changing the type of our new added create parameter from EnumSet to EnumSetWritable?
          dhruba borthakur made changes -
          Assignee He Yongqiang [ he yongqiang ]
          Tsz Wo Nicholas Sze made changes -
          Component/s io [ 12310687 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I think we should not change ObjectWritable here. We probably should add a new class EnumSetWritable, which is our usual approach.

          Show
          Tsz Wo Nicholas Sze added a comment - I think we should not change ObjectWritable here. We probably should add a new class EnumSetWritable, which is our usual approach.
          Hide
          He Yongqiang added a comment -

          The core unit test failuer seems not related to this patch.
          From the build's console output, we can see some errors went to c++/utils/impl/StringUtils

          Show
          He Yongqiang added a comment - The core unit test failuer seems not related to this patch. From the build's console output, we can see some errors went to c++/utils/impl/StringUtils
          He Yongqiang made changes -
          Summary Refactor ObjectWritable to support EnumSet Make ObjectWritable support EnumSet
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12404496/Hadoop-5596-2009-04-03.patch
          against trunk revision 761482.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12404496/Hadoop-5596-2009-04-03.patch against trunk revision 761482. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/105/console This message is automatically generated.
          He Yongqiang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          He Yongqiang added a comment -

          Hadoop-5596-2009-04-03.patch fixes the findbugs warning.

          Show
          He Yongqiang added a comment - Hadoop-5596-2009-04-03.patch fixes the findbugs warning.
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-04-03.patch [ 12404496 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12404208/Hadoop-5596-2009-03-31.patch
          against trunk revision 761082.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12404208/Hadoop-5596-2009-03-31.patch against trunk revision 761082. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/101/console This message is automatically generated.
          He Yongqiang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          He Yongqiang made changes -
          Attachment Hadoop-5596-2009-03-31.patch [ 12404208 ]
          Hide
          He Yongqiang added a comment -

          A quick fix for testing hadoop-5438.

          Show
          He Yongqiang added a comment - A quick fix for testing hadoop-5438.
          He Yongqiang made changes -
          Field Original Value New Value
          Link This issue blocks HADOOP-5438 [ HADOOP-5438 ]
          He Yongqiang created issue -

            People

            • Assignee:
              He Yongqiang
              Reporter:
              He Yongqiang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development