Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-9732

Improve DelegationTokenIdentifier.toString() for better logging

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.2
    • Fix Version/s: 3.0.0-alpha1
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      HDFS DelegationTokenIdentifier.toString() adds some diagnostics info, owner, sequence number. But its superclass, AbstractDelegationTokenIdentifier contains a lot more information, including token issue and expiry times.

      Because DelegationTokenIdentifier.toString() doesn't include this data,
      information that is potentially useful for kerberos diagnostics is lost.

      1. HADOOP-12752-001.patch
        3 kB
        Steve Loughran
      2. HDFS-9732.001.patch
        8 kB
        Yongjun Zhang
      3. HDFS-9732.002.patch
        9 kB
        Yongjun Zhang
      4. HDFS-9732.003.patch
        9 kB
        Yongjun Zhang
      5. HDFS-9732.004.patch
        10 kB
        Yongjun Zhang
      6. HDFS-9732-000.patch
        1 kB
        Steve Loughran

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          Code in the HDFS subclass

            @Override
            public String toString() {
              return getKind() + " token " + getSequenceNumber()
                  + " for " + getUser().getShortUserName() + " with renewer " +
                  getRenewer();
            }
          

          compared to that in superclass

              buffer
                  .append("owner=" + owner + ", renewer=" + renewer + ", realUser="
                      + realUser + ", issueDate=" + issueDate + ", maxDate=" + maxDate
                      + ", sequenceNumber=" + sequenceNumber + ", masterKeyId="
                      + masterKeyId);
          

          The code in AbstractDelegationTokenIdentifier is therefore a proper superclass of what the HDFS subclass prints. The best and simplest way to improve the diagnostics here is to remove the subclass's toString() method entirely

          Show
          stevel@apache.org Steve Loughran added a comment - Code in the HDFS subclass @Override public String toString() { return getKind() + " token " + getSequenceNumber() + " for " + getUser().getShortUserName() + " with renewer " + getRenewer(); } compared to that in superclass buffer .append( "owner=" + owner + ", renewer=" + renewer + ", realUser=" + realUser + ", issueDate=" + issueDate + ", maxDate=" + maxDate + ", sequenceNumber=" + sequenceNumber + ", masterKeyId=" + masterKeyId); The code in AbstractDelegationTokenIdentifier is therefore a proper superclass of what the HDFS subclass prints. The best and simplest way to improve the diagnostics here is to remove the subclass's toString() method entirely
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Cut the text. No attempt to write a test as there's not much to do except have something brittle to changes in string output

          Show
          stevel@apache.org Steve Loughran added a comment - Cut the text. No attempt to write a test as there's not much to do except have something brittle to changes in string output
          Hide
          cnauroth Chris Nauroth added a comment -

          The hdfs fetchdt command depends on the current output format of DelegationTokenIdentifier#toString. Unfortunately, that means the simple way to make this change also would be backwards-incompatible.

          We'll either need to target the change to 3.x, or look at breaking the dependency of hdfs fetchdt so that it maintains its current output, but toString can still add more detail for things like logging.

          HDFS-9085 is a patch that changed this code recently. The decision on that one was to go to trunk only.

          Show
          cnauroth Chris Nauroth added a comment - The hdfs fetchdt command depends on the current output format of DelegationTokenIdentifier#toString . Unfortunately, that means the simple way to make this change also would be backwards-incompatible. We'll either need to target the change to 3.x, or look at breaking the dependency of hdfs fetchdt so that it maintains its current output, but toString can still add more detail for things like logging. HDFS-9085 is a patch that changed this code recently. The decision on that one was to go to trunk only.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          And for comparison, here is the log of an HDFS token, compared to that of a timeline one, the latter not overriding {{ AbstractDelegationTokenIdentifier.toString()}}

          16/01/28 10:58:39 INFO mapreduce.JobSubmitter: Kind: HDFS_DELEGATION_TOKEN, Service: 192.168.96.154:8020, Ident: (HDFS_DELEGATION_TOKEN token 9 for stevel)
          
          16/01/28 10:18:19 DEBUG security.SecurityUtil: Acquired token Kind: TIMELINE_DELEGATION_TOKEN, Service: 192.168.96.154:8188, Ident: (owner=stevel, renewer=yarn, realUser=, issueDate=1453976299361, maxDate=1454581099361, sequenceNumber=6, masterKeyId=4)
          

          Note how much more useful the timeline token's one is.

          Show
          stevel@apache.org Steve Loughran added a comment - And for comparison, here is the log of an HDFS token, compared to that of a timeline one, the latter not overriding {{ AbstractDelegationTokenIdentifier.toString()}} 16/01/28 10:58:39 INFO mapreduce.JobSubmitter: Kind: HDFS_DELEGATION_TOKEN, Service: 192.168.96.154:8020, Ident: (HDFS_DELEGATION_TOKEN token 9 for stevel) 16/01/28 10:18:19 DEBUG security.SecurityUtil: Acquired token Kind: TIMELINE_DELEGATION_TOKEN, Service: 192.168.96.154:8188, Ident: (owner=stevel, renewer=yarn, realUser=, issueDate=1453976299361, maxDate=1454581099361, sequenceNumber=6, masterKeyId=4) Note how much more useful the timeline token's one is.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          0 mvndep 0m 8s Maven dependency ordering for branch
          +1 mvninstall 6m 41s trunk passed
          +1 compile 6m 17s trunk passed with JDK v1.8.0_66
          +1 compile 6m 59s trunk passed with JDK v1.7.0_91
          +1 checkstyle 0m 22s trunk passed
          +1 mvnsite 1m 3s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 0m 55s trunk passed with JDK v1.8.0_66
          +1 javadoc 1m 5s trunk passed with JDK v1.7.0_91
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 0m 40s the patch passed
          +1 compile 6m 17s the patch passed with JDK v1.8.0_66
          +1 javac 6m 17s the patch passed
          +1 compile 6m 59s the patch passed with JDK v1.7.0_91
          +1 javac 6m 59s the patch passed
          +1 checkstyle 0m 22s the patch passed
          +1 mvnsite 1m 3s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 2m 5s the patch passed
          +1 javadoc 0m 54s the patch passed with JDK v1.8.0_66
          +1 javadoc 1m 5s the patch passed with JDK v1.7.0_91
          +1 unit 6m 46s hadoop-common in the patch passed with JDK v1.8.0_66.
          +1 unit 6m 55s hadoop-common in the patch passed with JDK v1.7.0_91.
          +1 asflicense 0m 23s Patch does not generate ASF License warnings.
          60m 36s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12785566/HADOOP-12752-001.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c6dcae193319 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 59a212b
          Default Java 1.7.0_91
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91
          findbugs v3.0.0
          JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14324/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Max memory used 77MB
          Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14324/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 8s Maven dependency ordering for branch +1 mvninstall 6m 41s trunk passed +1 compile 6m 17s trunk passed with JDK v1.8.0_66 +1 compile 6m 59s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 22s trunk passed +1 mvnsite 1m 3s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 0m 55s trunk passed with JDK v1.8.0_66 +1 javadoc 1m 5s trunk passed with JDK v1.7.0_91 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 0m 40s the patch passed +1 compile 6m 17s the patch passed with JDK v1.8.0_66 +1 javac 6m 17s the patch passed +1 compile 6m 59s the patch passed with JDK v1.7.0_91 +1 javac 6m 59s the patch passed +1 checkstyle 0m 22s the patch passed +1 mvnsite 1m 3s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 2m 5s the patch passed +1 javadoc 0m 54s the patch passed with JDK v1.8.0_66 +1 javadoc 1m 5s the patch passed with JDK v1.7.0_91 +1 unit 6m 46s hadoop-common in the patch passed with JDK v1.8.0_66. +1 unit 6m 55s hadoop-common in the patch passed with JDK v1.7.0_91. +1 asflicense 0m 23s Patch does not generate ASF License warnings. 60m 36s Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12785566/HADOOP-12752-001.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c6dcae193319 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 59a212b Default Java 1.7.0_91 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91 findbugs v3.0.0 JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14324/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Max memory used 77MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14324/console This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This is a pain; I was just seeing that the debug logs weren't meaningful, now it turns out that fetchdt depends on it.

          We could always change how fetchdt generates that string, have one for end users and one for debugging. Though ideally, you'd have the end user app deciding what to extract and format itself, via some sprintf format string.

          Show
          stevel@apache.org Steve Loughran added a comment - This is a pain; I was just seeing that the debug logs weren't meaningful, now it turns out that fetchdt depends on it. We could always change how fetchdt generates that string, have one for end users and one for debugging. Though ideally, you'd have the end user app deciding what to extract and format itself, via some sprintf format string.
          Hide
          cnauroth Chris Nauroth added a comment -

          Yes, agreed. I would be +1 for an approach that converts hdfs fetchdt to use its own specific string formatting logic. I think reliance on toString in any public contract is generally a bad idea for the same reasons you described. People expect toString to be useful primarily for debugging, and they want it to be easily evolved to add more information.

          BTW, was the current attachment meant to go on a different JIRA?

          Show
          cnauroth Chris Nauroth added a comment - Yes, agreed. I would be +1 for an approach that converts hdfs fetchdt to use its own specific string formatting logic. I think reliance on toString in any public contract is generally a bad idea for the same reasons you described. People expect toString to be useful primarily for debugging, and they want it to be easily evolved to add more information. BTW, was the current attachment meant to go on a different JIRA?
          Hide
          aw Allen Wittenauer added a comment -

          Why do we have such limited output for users though? If I do, say, a klist -v, I get the full output. Why shouldn't fetchdt and/or dtutil?

          Show
          aw Allen Wittenauer added a comment - Why do we have such limited output for users though? If I do, say, a klist -v, I get the full output. Why shouldn't fetchdt and/or dtutil?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          you are right: wrong JIRA or diff. cancelling

          Show
          stevel@apache.org Steve Loughran added a comment - you are right: wrong JIRA or diff. cancelling
          Hide
          cnauroth Chris Nauroth added a comment -

          Why do we have such limited output for users though? If I do, say, a klist -v, I get the full output. Why shouldn't fetchdt and/or dtutil?

          I have no objection to providing richer output for end users. I just wanted to point out that there is a backwards-compatibility concern, at least within 2.x. We could probably add anything we want if it was hidden behind a --verbose flag or similar, and the default behavior retained the current output.

          Show
          cnauroth Chris Nauroth added a comment - Why do we have such limited output for users though? If I do, say, a klist -v, I get the full output. Why shouldn't fetchdt and/or dtutil? I have no objection to providing richer output for end users. I just wanted to point out that there is a backwards-compatibility concern, at least within 2.x. We could probably add anything we want if it was hidden behind a --verbose flag or similar, and the default behavior retained the current output.
          Hide
          aw Allen Wittenauer added a comment -

          I just wanted to point out that there is a backwards-compatibility concern, at least within 2.x.

          Who uses that 5 year old branch?

          Show
          aw Allen Wittenauer added a comment - I just wanted to point out that there is a backwards-compatibility concern, at least within 2.x. Who uses that 5 year old branch?
          Hide
          yzhangal Yongjun Zhang added a comment - - edited

          Hi Steve Loughran, thanks for reporting the issue here, and thanks Chris Nauroth and Allen Wittenauer for the discussion.

          I recently had an issue and reported HDFS-10211, Chris pointed out it's a duplicate of this one. The attached patch seems for other issue. Do you mind I work on this one?

          As Chris stated in the comment above, to be backward compatible, we can add --verbose switch to fetchdt to print out more details, but let the detail behavior to be the same as before.

          For places like to one I reported in HDFS-10211, I'd like to change it to print verbose info.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - - edited Hi Steve Loughran , thanks for reporting the issue here, and thanks Chris Nauroth and Allen Wittenauer for the discussion. I recently had an issue and reported HDFS-10211 , Chris pointed out it's a duplicate of this one. The attached patch seems for other issue. Do you mind I work on this one? As Chris stated in the comment above, to be backward compatible, we can add --verbose switch to fetchdt to print out more details, but let the detail behavior to be the same as before. For places like to one I reported in HDFS-10211 , I'd like to change it to print verbose info. Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Steve Loughran,

          I assume you don't mind that I work on this issue, and just assigned to myself Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Steve Loughran , I assume you don't mind that I work on this issue, and just assigned to myself Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Chris Nauroth and Steve Loughran,

          I just uploaded a patch, that's intended to be backward compatible and fix the issue here.

          Would you please help taking a look?

          Thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Chris Nauroth and Steve Loughran , I just uploaded a patch, that's intended to be backward compatible and fix the issue here. Would you please help taking a look? Thanks a lot.
          Hide
          aw Allen Wittenauer added a comment -

          To quote http://hadoop.apache.org/docs/r2.7.2/hadoop-project-dist/hadoop-common/Compatibility.html#Command_Line_Interface_CLI

          The Hadoop command line programs may be use either directly via the system shell or via shell scripts. Changing the path of a command, removing or renaming command line options, the order of arguments, or the command return code and output break compatibility and may adversely affect users.
          

          which means:

          When backward compatibility is not needed, use toString(), which provides
           more info and is supposed to evolve, see HDFS-9732.
          

          the toString method can pretty much never be called outside of logfiles. It's also very likely be to called incorrectly. I'd much rather see the frozen version be the normal toString and the 'flexible' one be something else.

          Show
          aw Allen Wittenauer added a comment - To quote http://hadoop.apache.org/docs/r2.7.2/hadoop-project-dist/hadoop-common/Compatibility.html#Command_Line_Interface_CLI The Hadoop command line programs may be use either directly via the system shell or via shell scripts. Changing the path of a command, removing or renaming command line options, the order of arguments, or the command return code and output break compatibility and may adversely affect users. which means: When backward compatibility is not needed, use toString(), which provides more info and is supposed to evolve, see HDFS-9732. the toString method can pretty much never be called outside of logfiles. It's also very likely be to called incorrectly. I'd much rather see the frozen version be the normal toString and the 'flexible' one be something else.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Allen Wittenauer.

          We often print out an object which implicitly calls toString(). It'd be annoying that the default doesn't print out all information. My thinking was, the default toString() is called more often than the places that have compatibility concern (One one place in this case), and letting the compatibility-sensitive places call Frozen version explicitly is also easier to read/understand.

          Does my thinking make sense to you?

          Thanks again.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Allen Wittenauer . We often print out an object which implicitly calls toString() . It'd be annoying that the default doesn't print out all information. My thinking was, the default toString() is called more often than the places that have compatibility concern (One one place in this case), and letting the compatibility-sensitive places call Frozen version explicitly is also easier to read/understand. Does my thinking make sense to you? Thanks again.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 57s Maven dependency ordering for branch
          +1 mvninstall 6m 41s trunk passed
          +1 compile 6m 0s trunk passed with JDK v1.8.0_74
          +1 compile 6m 52s trunk passed with JDK v1.7.0_95
          +1 checkstyle 1m 5s trunk passed
          +1 mvnsite 2m 23s trunk passed
          +1 mvneclipse 0m 41s trunk passed
          +1 findbugs 5m 15s trunk passed
          +1 javadoc 2m 20s trunk passed with JDK v1.8.0_74
          +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 2m 0s the patch passed
          +1 compile 5m 58s the patch passed with JDK v1.8.0_74
          +1 javac 5m 58s the patch passed
          +1 compile 6m 51s the patch passed with JDK v1.7.0_95
          +1 javac 6m 51s the patch passed
          +1 checkstyle 1m 5s the patch passed
          +1 mvnsite 2m 25s the patch passed
          +1 mvneclipse 0m 40s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 6m 0s the patch passed
          +1 javadoc 2m 16s the patch passed with JDK v1.8.0_74
          +1 javadoc 3m 18s the patch passed with JDK v1.7.0_95
          -1 unit 6m 29s hadoop-common in the patch failed with JDK v1.8.0_74.
          +1 unit 0m 52s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74.
          -1 unit 57m 25s hadoop-hdfs in the patch failed with JDK v1.8.0_74.
          -1 unit 7m 18s hadoop-common in the patch failed with JDK v1.7.0_95.
          +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95.
          -1 unit 56m 4s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
          -1 asflicense 0m 25s Patch generated 3 ASF License warnings.
          197m 35s



          Reason Tests
          JDK v1.8.0_74 Failed junit tests hadoop.net.TestDNS
            hadoop.hdfs.security.TestDelegationToken
            hadoop.hdfs.server.namenode.TestEditLog
            hadoop.hdfs.TestHFlush
          JDK v1.8.0_74 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker
          JDK v1.7.0_95 Failed junit tests hadoop.hdfs.security.TestDelegationToken
            hadoop.hdfs.TestHFlush
            hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
          JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795489/HDFS-9732.001.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 49dd30192126 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fde8ac5
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14947/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-asflicense-problems.txt
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14947/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 57s Maven dependency ordering for branch +1 mvninstall 6m 41s trunk passed +1 compile 6m 0s trunk passed with JDK v1.8.0_74 +1 compile 6m 52s trunk passed with JDK v1.7.0_95 +1 checkstyle 1m 5s trunk passed +1 mvnsite 2m 23s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 5m 15s trunk passed +1 javadoc 2m 20s trunk passed with JDK v1.8.0_74 +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 2m 0s the patch passed +1 compile 5m 58s the patch passed with JDK v1.8.0_74 +1 javac 5m 58s the patch passed +1 compile 6m 51s the patch passed with JDK v1.7.0_95 +1 javac 6m 51s the patch passed +1 checkstyle 1m 5s the patch passed +1 mvnsite 2m 25s the patch passed +1 mvneclipse 0m 40s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 6m 0s the patch passed +1 javadoc 2m 16s the patch passed with JDK v1.8.0_74 +1 javadoc 3m 18s the patch passed with JDK v1.7.0_95 -1 unit 6m 29s hadoop-common in the patch failed with JDK v1.8.0_74. +1 unit 0m 52s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74. -1 unit 57m 25s hadoop-hdfs in the patch failed with JDK v1.8.0_74. -1 unit 7m 18s hadoop-common in the patch failed with JDK v1.7.0_95. +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95. -1 unit 56m 4s hadoop-hdfs in the patch failed with JDK v1.7.0_95. -1 asflicense 0m 25s Patch generated 3 ASF License warnings. 197m 35s Reason Tests JDK v1.8.0_74 Failed junit tests hadoop.net.TestDNS   hadoop.hdfs.security.TestDelegationToken   hadoop.hdfs.server.namenode.TestEditLog   hadoop.hdfs.TestHFlush JDK v1.8.0_74 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker JDK v1.7.0_95 Failed junit tests hadoop.hdfs.security.TestDelegationToken   hadoop.hdfs.TestHFlush   hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795489/HDFS-9732.001.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 49dd30192126 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fde8ac5 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14947/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14947/artifact/patchprocess/patch-asflicense-problems.txt modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14947/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          The thing about those logfiles is they are the ones making more use of it, they are the ones where you only look at them when things have gone wrong and you want as much detail as you want. There's also the tradition of expanding toString diagnostics in subclasses; here subclasses would need to know to not call super.toString() and instead call some other method

          Update that and alongside it add a public @stable method , say detailsForCLI() javadoced as "do not change this output". A unique name and text will keep anyone from adding it to it later.

          I absolutely do not want to break CLI output here —and I'm glad you picked up on it— all we need to do know is work out the way to both improve log diagnostics and ensure that nobody else tries to improve the output later

          Show
          stevel@apache.org Steve Loughran added a comment - The thing about those logfiles is they are the ones making more use of it, they are the ones where you only look at them when things have gone wrong and you want as much detail as you want. There's also the tradition of expanding toString diagnostics in subclasses; here subclasses would need to know to not call super.toString() and instead call some other method Update that and alongside it add a public @stable method , say detailsForCLI() javadoced as "do not change this output". A unique name and text will keep anyone from adding it to it later. I absolutely do not want to break CLI output here —and I'm glad you picked up on it— all we need to do know is work out the way to both improve log diagnostics and ensure that nobody else tries to improve the output later
          Hide
          cnauroth Chris Nauroth added a comment -

          I agree with the approach of using toString as the method for detailed debugging information that may freely evolve as we think of more helpful troubleshooting tips. hdfs fetchdt can move to another dedicated method, with output that must adhere to the compatibility policy.

          In general, I think developers expect the primary use case for toString to be debugging and logging of object innards. I consider it poor practice to use toString for any kind of user-facing object serialization. I've seen projects that go as far as forbidding use of toString for serialization.

          Show
          cnauroth Chris Nauroth added a comment - I agree with the approach of using toString as the method for detailed debugging information that may freely evolve as we think of more helpful troubleshooting tips. hdfs fetchdt can move to another dedicated method, with output that must adhere to the compatibility policy. In general, I think developers expect the primary use case for toString to be debugging and logging of object innards. I consider it poor practice to use toString for any kind of user-facing object serialization. I've seen projects that go as far as forbidding use of toString for serialization.
          Hide
          yzhangal Yongjun Zhang added a comment -

          New patch rev 002 to fix a test failure and with some massage.

          Show
          yzhangal Yongjun Zhang added a comment - New patch rev 002 to fix a test failure and with some massage.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Chris Nauroth, did not see your comment until now. I hope Allen Wittenauer would agree too. Thanks Allen.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Chris Nauroth , did not see your comment until now. I hope Allen Wittenauer would agree too. Thanks Allen.
          Hide
          aw Allen Wittenauer added a comment - - edited

          While I can appreciate what you folks are saying, experience has shown that Hadoop devs are not disciplined enough to use this appropriately. Incredibly bad code gets tossed over the fence all the time that are obviously bad--my current favorite for 2.8 is still the "log metrics to files outside of the metrics subsystem via log4j rather than just fixing the file metrics plug-in". Something subtle like this difference is going to blow up big time. I still believe that it should be harder to do the wrong thing, but I'll acquiesce in order to move this forward.

          That said, I'd still like to see:

          a) audit every direct and indirect usage of toString to make sure it isn't getting used for CLI output
          b) the javadoc for the toString method needs to explicitly say that it is not to be used for CLI output because it evolves and point to the relevant section in the compat guidelines
          c) the toStringFrozen should be renamed toStringCLI or something similar to actually state what it does not what it is so that in 3.x it can be changed.

          Show
          aw Allen Wittenauer added a comment - - edited While I can appreciate what you folks are saying, experience has shown that Hadoop devs are not disciplined enough to use this appropriately. Incredibly bad code gets tossed over the fence all the time that are obviously bad--my current favorite for 2.8 is still the "log metrics to files outside of the metrics subsystem via log4j rather than just fixing the file metrics plug-in". Something subtle like this difference is going to blow up big time. I still believe that it should be harder to do the wrong thing, but I'll acquiesce in order to move this forward. That said, I'd still like to see: a) audit every direct and indirect usage of toString to make sure it isn't getting used for CLI output b) the javadoc for the toString method needs to explicitly say that it is not to be used for CLI output because it evolves and point to the relevant section in the compat guidelines c) the toStringFrozen should be renamed toStringCLI or something similar to actually state what it does not what it is so that in 3.x it can be changed.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran,

          Ah, sorry, thanks for your comments, I did not see until now,

          About the method name, detailsForCLI(), do we have any other occasion that we want a "frozen" output then CLI? For example, one day we need to call the frozen version for webui? If CLI is the only situation, I can change the new method name per your suggestion.

          One question:

          here subclasses would need to know to not call super.toString() and instead call some other method

          Derived class may have new field to print on top of the base class'. If we don't call super.toString(), we probably introduce a new method to be called by both the base class' and child class' toString()?

          In this case, all the info printed by the base toString() is applicable to child class. And in my rev 2, I added getKind() which is overriden by child classes. Basically we are in full control of what the output looks like. Would you please explain why calling super.toString() is a bad idea (especially we do define our own base class' toString())? I can see a problem when we don't define toString() class for the base, in which case the java base Object's toString() would be called.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Ah, sorry, thanks for your comments, I did not see until now, About the method name, detailsForCLI() , do we have any other occasion that we want a "frozen" output then CLI? For example, one day we need to call the frozen version for webui? If CLI is the only situation, I can change the new method name per your suggestion. One question: here subclasses would need to know to not call super.toString() and instead call some other method Derived class may have new field to print on top of the base class'. If we don't call super.toString() , we probably introduce a new method to be called by both the base class' and child class' toString() ? In this case, all the info printed by the base toString() is applicable to child class. And in my rev 2, I added getKind() which is overriden by child classes. Basically we are in full control of what the output looks like. Would you please explain why calling super.toString() is a bad idea (especially we do define our own base class' toString() )? I can see a problem when we don't define toString() class for the base, in which case the java base Object 's toString() would be called. Thanks.
          Hide
          aw Allen Wittenauer added a comment -

          do we have any other occasion that we want a "frozen" output then CLI?

          Double check with the compat guidelines, but I'm fairly confident that the only output that can't change is the CLI. Web output, for example, is specifically called out as Unstable. (Because if it wasn't, the deadly web UI redo in 2.5/2.6 that basically screwed over most of the people with security deployed wouldn't have went in.)

          Show
          aw Allen Wittenauer added a comment - do we have any other occasion that we want a "frozen" output then CLI? Double check with the compat guidelines, but I'm fairly confident that the only output that can't change is the CLI. Web output, for example, is specifically called out as Unstable. (Because if it wasn't, the deadly web UI redo in 2.5/2.6 that basically screwed over most of the people with security deployed wouldn't have went in.)
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 6m 38s trunk passed
          +1 compile 5m 53s trunk passed with JDK v1.8.0_74
          +1 compile 6m 38s trunk passed with JDK v1.7.0_95
          +1 checkstyle 1m 4s trunk passed
          +1 mvnsite 2m 21s trunk passed
          +1 mvneclipse 0m 42s trunk passed
          +1 findbugs 5m 5s trunk passed
          +1 javadoc 2m 18s trunk passed with JDK v1.8.0_74
          +1 javadoc 3m 13s trunk passed with JDK v1.7.0_95
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 2m 1s the patch passed
          +1 compile 5m 48s the patch passed with JDK v1.8.0_74
          +1 javac 5m 48s the patch passed
          +1 compile 6m 42s the patch passed with JDK v1.7.0_95
          +1 javac 6m 42s the patch passed
          +1 checkstyle 1m 4s the patch passed
          +1 mvnsite 2m 20s the patch passed
          +1 mvneclipse 0m 40s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 5m 46s the patch passed
          +1 javadoc 2m 19s the patch passed with JDK v1.8.0_74
          +1 javadoc 3m 14s the patch passed with JDK v1.7.0_95
          -1 unit 6m 29s hadoop-common in the patch failed with JDK v1.8.0_74.
          +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74.
          -1 unit 67m 44s hadoop-hdfs in the patch failed with JDK v1.8.0_74.
          -1 unit 6m 50s hadoop-common in the patch failed with JDK v1.7.0_95.
          +1 unit 0m 59s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95.
          -1 unit 67m 30s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
          -1 asflicense 0m 27s Patch generated 3 ASF License warnings.
          216m 58s



          Reason Tests
          JDK v1.8.0_74 Failed junit tests hadoop.hdfs.TestHFlush
            hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot
            hadoop.hdfs.TestRollingUpgrade
          JDK v1.8.0_74 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker
          JDK v1.7.0_95 Failed junit tests hadoop.hdfs.TestDFSUpgradeFromImage
            hadoop.hdfs.TestRollingUpgrade
          JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795519/HDFS-9732.002.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux cf4909ca444b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / a337ceb
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14950/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-asflicense-problems.txt
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14950/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 6m 38s trunk passed +1 compile 5m 53s trunk passed with JDK v1.8.0_74 +1 compile 6m 38s trunk passed with JDK v1.7.0_95 +1 checkstyle 1m 4s trunk passed +1 mvnsite 2m 21s trunk passed +1 mvneclipse 0m 42s trunk passed +1 findbugs 5m 5s trunk passed +1 javadoc 2m 18s trunk passed with JDK v1.8.0_74 +1 javadoc 3m 13s trunk passed with JDK v1.7.0_95 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 2m 1s the patch passed +1 compile 5m 48s the patch passed with JDK v1.8.0_74 +1 javac 5m 48s the patch passed +1 compile 6m 42s the patch passed with JDK v1.7.0_95 +1 javac 6m 42s the patch passed +1 checkstyle 1m 4s the patch passed +1 mvnsite 2m 20s the patch passed +1 mvneclipse 0m 40s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 5m 46s the patch passed +1 javadoc 2m 19s the patch passed with JDK v1.8.0_74 +1 javadoc 3m 14s the patch passed with JDK v1.7.0_95 -1 unit 6m 29s hadoop-common in the patch failed with JDK v1.8.0_74. +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74. -1 unit 67m 44s hadoop-hdfs in the patch failed with JDK v1.8.0_74. -1 unit 6m 50s hadoop-common in the patch failed with JDK v1.7.0_95. +1 unit 0m 59s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95. -1 unit 67m 30s hadoop-hdfs in the patch failed with JDK v1.7.0_95. -1 asflicense 0m 27s Patch generated 3 ASF License warnings. 216m 58s Reason Tests JDK v1.8.0_74 Failed junit tests hadoop.hdfs.TestHFlush   hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot   hadoop.hdfs.TestRollingUpgrade JDK v1.8.0_74 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker JDK v1.7.0_95 Failed junit tests hadoop.hdfs.TestDFSUpgradeFromImage   hadoop.hdfs.TestRollingUpgrade JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.util.TestNativeLibraryChecker Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795519/HDFS-9732.002.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux cf4909ca444b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a337ceb Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_74.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14950/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14950/artifact/patchprocess/patch-asflicense-problems.txt modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14950/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment - - edited

          Thanks Allen Wittenauer.

          I like your idea of checking over all places about the use of toString() in CLI outputs. However, I think we can do it in separate jira as a master jira. And let each subtask fix some related commands, or even fix one particular issue (like this one). It would be really ambitious to use one jira to cover all IMHO. But good thing is, we are aware of that this has been an long-standing issue based on the discussion here, and we need keep this issue in mind when reviewing jiras that may incur output incompatibility.

          About method naming. Wonder if we can do toStringFrozen() or toStringStable() and javadoc it as "stable API for backward compatibility, currently only for CLI"? I can see that if we put the CLI keyword in the name, then calling these methods for anything else would look awkward, I guess there is still chance of calling these methods for something else.

          The point we are trying to make is, the method need to stable, and do not change it otherwise compatibility will be broken; and it's not just because they are for CLI. For example, some methods called by CLI don't need to be stable at all.

          Probably we use name toStringStable() instead?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - - edited Thanks Allen Wittenauer . I like your idea of checking over all places about the use of toString() in CLI outputs. However, I think we can do it in separate jira as a master jira. And let each subtask fix some related commands, or even fix one particular issue (like this one). It would be really ambitious to use one jira to cover all IMHO. But good thing is, we are aware of that this has been an long-standing issue based on the discussion here, and we need keep this issue in mind when reviewing jiras that may incur output incompatibility. About method naming. Wonder if we can do toStringFrozen() or toStringStable() and javadoc it as "stable API for backward compatibility, currently only for CLI"? I can see that if we put the CLI keyword in the name, then calling these methods for anything else would look awkward, I guess there is still chance of calling these methods for something else. The point we are trying to make is, the method need to stable , and do not change it otherwise compatibility will be broken; and it's not just because they are for CLI. For example, some methods called by CLI don't need to be stable at all. Probably we use name toStringStable() instead? Thanks.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          filing my original patch as -000; this just cuts the method.

          Show
          stevel@apache.org Steve Loughran added a comment - filing my original patch as -000; this just cuts the method.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          audit every direct and indirect usage of toString to make sure it isn't getting used for CLI output

          that'd take forever. Better to go from CLI entry points and their outputs.

          b) the javadoc for the toString method needs to explicitly say that it is not to be used for CLI output because it evolves and point to the relevant section in the compat guidelines

          Maybe the compat guidelines say "toString() is flexible", but toStringCLI( is frozen. Make that an interface too for easy tracking down of use. You could also have a special println(ToStringCLI instance) which would call and print it.

          c) the toStringFrozen should be renamed toStringCLI or something similar to actually state what it does not what it is so that in 3.x it can be changed.

          +1

          Show
          stevel@apache.org Steve Loughran added a comment - audit every direct and indirect usage of toString to make sure it isn't getting used for CLI output that'd take forever. Better to go from CLI entry points and their outputs. b) the javadoc for the toString method needs to explicitly say that it is not to be used for CLI output because it evolves and point to the relevant section in the compat guidelines Maybe the compat guidelines say "toString() is flexible", but toStringCLI( is frozen. Make that an interface too for easy tracking down of use. You could also have a special println(ToStringCLI instance) which would call and print it. c) the toStringFrozen should be renamed toStringCLI or something similar to actually state what it does not what it is so that in 3.x it can be changed. +1
          Hide
          stevel@apache.org Steve Loughran added a comment -
          1. I'd like the toCLI as it makes it clear this is for the CLI., but I see your reasoning. Let's see what others think.
          2. see what Ive proposed about having an interface for this.

          Regarding patch 002

          • Is there any way to reuse the mock instance. MiniDFS clusters kill test run performance, and are now expected to be a @BeforeClass instance, if used at all. How about just creating a token from some static inputs?
          • ...which you can then compare against a golden output string to verify that the output really does match expectations
          Show
          stevel@apache.org Steve Loughran added a comment - I'd like the toCLI as it makes it clear this is for the CLI., but I see your reasoning. Let's see what others think. see what Ive proposed about having an interface for this. Regarding patch 002 Is there any way to reuse the mock instance. MiniDFS clusters kill test run performance, and are now expected to be a @BeforeClass instance, if used at all. How about just creating a token from some static inputs? ...which you can then compare against a golden output string to verify that the output really does match expectations
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 9s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 6m 40s trunk passed
          +1 compile 0m 27s trunk passed with JDK v1.8.0_74
          +1 compile 0m 28s trunk passed with JDK v1.7.0_95
          +1 checkstyle 0m 17s trunk passed
          +1 mvnsite 0m 34s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 38s trunk passed
          +1 javadoc 0m 20s trunk passed with JDK v1.8.0_74
          +1 javadoc 0m 24s trunk passed with JDK v1.7.0_95
          +1 mvninstall 0m 30s the patch passed
          +1 compile 0m 24s the patch passed with JDK v1.8.0_74
          +1 javac 0m 24s the patch passed
          +1 compile 0m 26s the patch passed with JDK v1.7.0_95
          +1 javac 0m 26s the patch passed
          +1 checkstyle 0m 14s the patch passed
          +1 mvnsite 0m 30s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 1m 49s the patch passed
          +1 javadoc 0m 18s the patch passed with JDK v1.8.0_74
          +1 javadoc 0m 22s the patch passed with JDK v1.7.0_95
          +1 unit 0m 47s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74.
          +1 unit 0m 54s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95.
          -1 asflicense 0m 18s Patch generated 1 ASF License warnings.
          19m 49s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795613/HDFS-9732-000.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux ad9f68c91a70 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 90fcb16
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14957/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14957/artifact/patchprocess/patch-asflicense-problems.txt
          modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14957/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 9s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 40s trunk passed +1 compile 0m 27s trunk passed with JDK v1.8.0_74 +1 compile 0m 28s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 17s trunk passed +1 mvnsite 0m 34s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 38s trunk passed +1 javadoc 0m 20s trunk passed with JDK v1.8.0_74 +1 javadoc 0m 24s trunk passed with JDK v1.7.0_95 +1 mvninstall 0m 30s the patch passed +1 compile 0m 24s the patch passed with JDK v1.8.0_74 +1 javac 0m 24s the patch passed +1 compile 0m 26s the patch passed with JDK v1.7.0_95 +1 javac 0m 26s the patch passed +1 checkstyle 0m 14s the patch passed +1 mvnsite 0m 30s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 18s the patch passed with JDK v1.8.0_74 +1 javadoc 0m 22s the patch passed with JDK v1.7.0_95 +1 unit 0m 47s hadoop-hdfs-client in the patch passed with JDK v1.8.0_74. +1 unit 0m 54s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95. -1 asflicense 0m 18s Patch generated 1 ASF License warnings. 19m 49s Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12795613/HDFS-9732-000.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ad9f68c91a70 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 90fcb16 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14957/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/14957/artifact/patchprocess/patch-asflicense-problems.txt modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14957/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran,

          Thanks a lot for your review and comments. Sorry for getting back late since I was out last week.

          I just uploaded rev 03 to address your comments:

          • I changed the method to toStringStable with java docs
          • I found using mock in this case is hard to create a valid DelegationTokenIdentifier to be parsed by AbstractDelegationTokenIdentifier#readFields, so I decided to plugin my test to an existing test that starts mini cluster
          • I introduced a new method visible for test to return a string, the method is called both in production to print out to System.out and can be checked in unit test

          Would you, Chris Nauroth and Allen Wittenauer please help to take a look at rev 03? thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Thanks a lot for your review and comments. Sorry for getting back late since I was out last week. I just uploaded rev 03 to address your comments: I changed the method to toStringStable with java docs I found using mock in this case is hard to create a valid DelegationTokenIdentifier to be parsed by AbstractDelegationTokenIdentifier#readFields , so I decided to plugin my test to an existing test that starts mini cluster I introduced a new method visible for test to return a string, the method is called both in production to print out to System.out and can be checked in unit test Would you, Chris Nauroth and Allen Wittenauer please help to take a look at rev 03? thanks a lot.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 11m 27s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 54s Maven dependency ordering for branch
          +1 mvninstall 6m 46s trunk passed
          +1 compile 5m 54s trunk passed with JDK v1.8.0_77
          +1 compile 6m 41s trunk passed with JDK v1.7.0_95
          +1 checkstyle 1m 5s trunk passed
          +1 mvnsite 2m 24s trunk passed
          +1 mvneclipse 0m 41s trunk passed
          +1 findbugs 5m 5s trunk passed
          +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77
          +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 56s the patch passed
          +1 compile 5m 51s the patch passed with JDK v1.8.0_77
          +1 javac 5m 51s the patch passed
          +1 compile 6m 36s the patch passed with JDK v1.7.0_95
          +1 javac 6m 36s the patch passed
          +1 checkstyle 1m 4s the patch passed
          +1 mvnsite 2m 23s the patch passed
          +1 mvneclipse 0m 40s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 5m 54s the patch passed
          +1 javadoc 2m 21s the patch passed with JDK v1.8.0_77
          +1 javadoc 3m 20s the patch passed with JDK v1.7.0_95
          -1 unit 6m 37s hadoop-common in the patch failed with JDK v1.8.0_77.
          +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77.
          -1 unit 55m 51s hadoop-hdfs in the patch failed with JDK v1.8.0_77.
          -1 unit 6m 54s hadoop-common in the patch failed with JDK v1.7.0_95.
          +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95.
          -1 unit 53m 13s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
          +1 asflicense 0m 26s Patch does not generate ASF License warnings.
          203m 12s



          Reason Tests
          JDK v1.8.0_77 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics
            hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
          JDK v1.7.0_95 Failed junit tests hadoop.net.TestDNS
            hadoop.hdfs.TestHFlush
            hadoop.hdfs.server.namenode.TestEditLog



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12796936/HDFS-9732.003.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 0977e37b62a7 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f65f5b1
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15058/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15058/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 11m 27s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 54s Maven dependency ordering for branch +1 mvninstall 6m 46s trunk passed +1 compile 5m 54s trunk passed with JDK v1.8.0_77 +1 compile 6m 41s trunk passed with JDK v1.7.0_95 +1 checkstyle 1m 5s trunk passed +1 mvnsite 2m 24s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 5m 5s trunk passed +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77 +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 56s the patch passed +1 compile 5m 51s the patch passed with JDK v1.8.0_77 +1 javac 5m 51s the patch passed +1 compile 6m 36s the patch passed with JDK v1.7.0_95 +1 javac 6m 36s the patch passed +1 checkstyle 1m 4s the patch passed +1 mvnsite 2m 23s the patch passed +1 mvneclipse 0m 40s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 5m 54s the patch passed +1 javadoc 2m 21s the patch passed with JDK v1.8.0_77 +1 javadoc 3m 20s the patch passed with JDK v1.7.0_95 -1 unit 6m 37s hadoop-common in the patch failed with JDK v1.8.0_77. +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77. -1 unit 55m 51s hadoop-hdfs in the patch failed with JDK v1.8.0_77. -1 unit 6m 54s hadoop-common in the patch failed with JDK v1.7.0_95. +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95. -1 unit 53m 13s hadoop-hdfs in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 26s Patch does not generate ASF License warnings. 203m 12s Reason Tests JDK v1.8.0_77 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics   hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl JDK v1.7.0_95 Failed junit tests hadoop.net.TestDNS   hadoop.hdfs.TestHFlush   hadoop.hdfs.server.namenode.TestEditLog Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12796936/HDFS-9732.003.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0977e37b62a7 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f65f5b1 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15058/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15058/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15058/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran, Chris Nauroth, Allen Wittenauer,

          Wonder if you could help taking a look at rev 03? thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Chris Nauroth , Allen Wittenauer , Wonder if you could help taking a look at rev 03? thanks a lot.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran, Chris Nauroth, Allen Wittenauer,

          I hope we can get this in asap, would you please help taking a look at rev 03? many thanks.

          --Yongjun

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Chris Nauroth , Allen Wittenauer , I hope we can get this in asap, would you please help taking a look at rev 03? many thanks. --Yongjun
          Hide
          cnauroth Chris Nauroth added a comment -

          Yongjun Zhang, I apologize for the delay. I do have this in my review queue, but that queue is very deep at the moment.

          Show
          cnauroth Chris Nauroth added a comment - Yongjun Zhang , I apologize for the delay. I do have this in my review queue, but that queue is very deep at the moment.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Chris Nauroth!

          My last rev mainly addressed Steve's comments, which I hope that he could help taking a look (would you please Steve Loughran?).

          The thing I hope you can quickly help is, do you agree my using toStringStable as a name in rev 03? If you can comment on that, that would be helpful enough. Please see my reasoning here

          https://issues.apache.org/jira/browse/HDFS-9732?focusedCommentId=15213210&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15213210

          Thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Chris Nauroth ! My last rev mainly addressed Steve's comments, which I hope that he could help taking a look (would you please Steve Loughran ?). The thing I hope you can quickly help is, do you agree my using toStringStable as a name in rev 03? If you can comment on that, that would be helpful enough. Please see my reasoning here https://issues.apache.org/jira/browse/HDFS-9732?focusedCommentId=15213210&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15213210 Thanks a lot.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Getting close, just some details

          in DelegationTokenFetcher

          1. good to see you are using StringBuilder, can you split the append() sequence in to one per entry. Thats in toStringStable and printTokensToString. If you use intellij IDEA, it'll do that automatically if you ask it nicely.
            #line 78, how about we make the text just "print verbose output"

          In TestDelegationTokenFetcher

          1. Line 141 assert statement should build up a string to print on failure. Imagine: everything you'd need to understand the problem from a jenkins test failure
          2. Lines 142-143 use SLF4J logging APIs, not System.out
          Show
          stevel@apache.org Steve Loughran added a comment - Getting close, just some details in DelegationTokenFetcher good to see you are using StringBuilder, can you split the append() sequence in to one per entry. Thats in toStringStable and printTokensToString . If you use intellij IDEA, it'll do that automatically if you ask it nicely. #line 78, how about we make the text just "print verbose output" In TestDelegationTokenFetcher Line 141 assert statement should build up a string to print on failure. Imagine: everything you'd need to understand the problem from a jenkins test failure Lines 142-143 use SLF4J logging APIs, not System.out
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran,

          Thanks a lot for your review, all good comments. I just uploaded rev 004 that tries to address all your comments. Would you please help taking a look when you have chance? thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Thanks a lot for your review, all good comments. I just uploaded rev 004 that tries to address all your comments. Would you please help taking a look when you have chance? thanks.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'm happy with this, you've done some great work here.

          Allen Wittenauer do you have any issues with the patch as is? Notice how there are even unit tests to verify the output is stable, which is something no other bit of the code has

          Show
          stevel@apache.org Steve Loughran added a comment - I'm happy with this, you've done some great work here. Allen Wittenauer do you have any issues with the patch as is? Notice how there are even unit tests to verify the output is stable, which is something no other bit of the code has
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 16s Maven dependency ordering for branch
          +1 mvninstall 7m 30s trunk passed
          +1 compile 6m 5s trunk passed with JDK v1.8.0_77
          +1 compile 6m 44s trunk passed with JDK v1.7.0_95
          +1 checkstyle 1m 4s trunk passed
          +1 mvnsite 2m 23s trunk passed
          +1 mvneclipse 0m 41s trunk passed
          +1 findbugs 5m 13s trunk passed
          +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77
          +1 javadoc 3m 17s trunk passed with JDK v1.7.0_95
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 59s the patch passed
          +1 compile 6m 39s the patch passed with JDK v1.8.0_77
          +1 javac 6m 39s the patch passed
          +1 compile 7m 2s the patch passed with JDK v1.7.0_95
          +1 javac 7m 2s the patch passed
          +1 checkstyle 1m 9s the patch passed
          +1 mvnsite 2m 24s the patch passed
          +1 mvneclipse 0m 39s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 6m 2s the patch passed
          +1 javadoc 2m 42s the patch passed with JDK v1.8.0_77
          +1 javadoc 3m 32s the patch passed with JDK v1.7.0_95
          -1 unit 19m 58s hadoop-common in the patch failed with JDK v1.8.0_77.
          +1 unit 1m 1s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77.
          -1 unit 67m 17s hadoop-hdfs in the patch failed with JDK v1.8.0_77.
          +1 unit 8m 1s hadoop-common in the patch passed with JDK v1.7.0_95.
          +1 unit 0m 57s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95.
          -1 unit 56m 19s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
          +1 asflicense 0m 24s Patch does not generate ASF License warnings.
          223m 38s



          Reason Tests
          JDK v1.8.0_77 Failed junit tests hadoop.hdfs.TestFileAppend
          JDK v1.8.0_77 Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle
          JDK v1.7.0_95 Failed junit tests hadoop.hdfs.server.namenode.TestEditLog
            hadoop.hdfs.TestHFlush
            hadoop.hdfs.server.namenode.TestStartup



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12800022/HDFS-9732.004.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4ee047f41a69 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 4838b73
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15243/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15243/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 16s Maven dependency ordering for branch +1 mvninstall 7m 30s trunk passed +1 compile 6m 5s trunk passed with JDK v1.8.0_77 +1 compile 6m 44s trunk passed with JDK v1.7.0_95 +1 checkstyle 1m 4s trunk passed +1 mvnsite 2m 23s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 5m 13s trunk passed +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77 +1 javadoc 3m 17s trunk passed with JDK v1.7.0_95 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 59s the patch passed +1 compile 6m 39s the patch passed with JDK v1.8.0_77 +1 javac 6m 39s the patch passed +1 compile 7m 2s the patch passed with JDK v1.7.0_95 +1 javac 7m 2s the patch passed +1 checkstyle 1m 9s the patch passed +1 mvnsite 2m 24s the patch passed +1 mvneclipse 0m 39s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 6m 2s the patch passed +1 javadoc 2m 42s the patch passed with JDK v1.8.0_77 +1 javadoc 3m 32s the patch passed with JDK v1.7.0_95 -1 unit 19m 58s hadoop-common in the patch failed with JDK v1.8.0_77. +1 unit 1m 1s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77. -1 unit 67m 17s hadoop-hdfs in the patch failed with JDK v1.8.0_77. +1 unit 8m 1s hadoop-common in the patch passed with JDK v1.7.0_95. +1 unit 0m 57s hadoop-hdfs-client in the patch passed with JDK v1.7.0_95. -1 unit 56m 19s hadoop-hdfs in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 24s Patch does not generate ASF License warnings. 223m 38s Reason Tests JDK v1.8.0_77 Failed junit tests hadoop.hdfs.TestFileAppend JDK v1.8.0_77 Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle JDK v1.7.0_95 Failed junit tests hadoop.hdfs.server.namenode.TestEditLog   hadoop.hdfs.TestHFlush   hadoop.hdfs.server.namenode.TestStartup Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12800022/HDFS-9732.004.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4ee047f41a69 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4838b73 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15243/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15243/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15243/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 17s Maven dependency ordering for branch
          +1 mvninstall 6m 52s trunk passed
          +1 compile 5m 56s trunk passed with JDK v1.8.0_77
          +1 compile 6m 45s trunk passed with JDK v1.7.0_95
          +1 checkstyle 1m 4s trunk passed
          +1 mvnsite 2m 24s trunk passed
          +1 mvneclipse 0m 40s trunk passed
          +1 findbugs 5m 13s trunk passed
          +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77
          +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 2m 2s the patch passed
          +1 compile 6m 1s the patch passed with JDK v1.8.0_77
          +1 javac 6m 1s the patch passed
          +1 compile 7m 25s the patch passed with JDK v1.7.0_95
          +1 javac 7m 25s the patch passed
          +1 checkstyle 1m 16s the patch passed
          +1 mvnsite 2m 44s the patch passed
          +1 mvneclipse 0m 45s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 6m 46s the patch passed
          +1 javadoc 2m 45s the patch passed with JDK v1.8.0_77
          +1 javadoc 3m 35s the patch passed with JDK v1.7.0_95
          -1 unit 8m 45s hadoop-common in the patch failed with JDK v1.8.0_77.
          +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77.
          -1 unit 64m 27s hadoop-hdfs in the patch failed with JDK v1.8.0_77.
          +1 unit 8m 19s hadoop-common in the patch passed with JDK v1.7.0_95.
          -1 unit 0m 31s hadoop-hdfs-client in the patch failed with JDK v1.7.0_95.
          -1 unit 0m 35s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
          +1 asflicense 0m 29s Patch does not generate ASF License warnings.
          154m 19s



          Reason Tests
          JDK v1.8.0_77 Failed junit tests hadoop.ha.TestZKFailoverController
            hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
            hadoop.hdfs.server.namenode.ha.TestEditLogTailer
            hadoop.hdfs.TestFileAppend
            hadoop.hdfs.security.TestDelegationTokenForProxyUser



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:fbe3e86
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12800022/HDFS-9732.004.patch
          JIRA Issue HDFS-9732
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a1d74b2a3503 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c610031
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_95.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15257/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15257/console
          Powered by Apache Yetus 0.2.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 17s Maven dependency ordering for branch +1 mvninstall 6m 52s trunk passed +1 compile 5m 56s trunk passed with JDK v1.8.0_77 +1 compile 6m 45s trunk passed with JDK v1.7.0_95 +1 checkstyle 1m 4s trunk passed +1 mvnsite 2m 24s trunk passed +1 mvneclipse 0m 40s trunk passed +1 findbugs 5m 13s trunk passed +1 javadoc 2m 20s trunk passed with JDK v1.8.0_77 +1 javadoc 3m 15s trunk passed with JDK v1.7.0_95 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 2m 2s the patch passed +1 compile 6m 1s the patch passed with JDK v1.8.0_77 +1 javac 6m 1s the patch passed +1 compile 7m 25s the patch passed with JDK v1.7.0_95 +1 javac 7m 25s the patch passed +1 checkstyle 1m 16s the patch passed +1 mvnsite 2m 44s the patch passed +1 mvneclipse 0m 45s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 6m 46s the patch passed +1 javadoc 2m 45s the patch passed with JDK v1.8.0_77 +1 javadoc 3m 35s the patch passed with JDK v1.7.0_95 -1 unit 8m 45s hadoop-common in the patch failed with JDK v1.8.0_77. +1 unit 0m 58s hadoop-hdfs-client in the patch passed with JDK v1.8.0_77. -1 unit 64m 27s hadoop-hdfs in the patch failed with JDK v1.8.0_77. +1 unit 8m 19s hadoop-common in the patch passed with JDK v1.7.0_95. -1 unit 0m 31s hadoop-hdfs-client in the patch failed with JDK v1.7.0_95. -1 unit 0m 35s hadoop-hdfs in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 29s Patch does not generate ASF License warnings. 154m 19s Reason Tests JDK v1.8.0_77 Failed junit tests hadoop.ha.TestZKFailoverController   hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl   hadoop.hdfs.server.namenode.ha.TestEditLogTailer   hadoop.hdfs.TestFileAppend   hadoop.hdfs.security.TestDelegationTokenForProxyUser Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12800022/HDFS-9732.004.patch JIRA Issue HDFS-9732 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a1d74b2a3503 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c610031 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_95.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15257/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15257/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15257/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks a lot Steve Loughran!

          I reran all the failed tests locally and they are all successful, except that TestFileAppend failure is intermittent and I created HDFS-10333 for it.

          Would you please see if you could +1 on the last rev you reviewed unless Allen Wittenauer has additional comments?

          Thanks again.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks a lot Steve Loughran ! I reran all the failed tests locally and they are all successful, except that TestFileAppend failure is intermittent and I created HDFS-10333 for it. Would you please see if you could +1 on the last rev you reviewed unless Allen Wittenauer has additional comments? Thanks again.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran, wonder if you could take a look at rev 004? Thank you!

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , wonder if you could take a look at rev 004? Thank you!
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for the patch. May I ask why the StringBuilder is preferred (though welcomed by above comment) to + operator in this patch? Thanks.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for the patch. May I ask why the StringBuilder is preferred (though welcomed by above comment) to + operator in this patch? Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Mingliang Liu,

          Thanks for your question. Please see some discussion here
          http://stackoverflow.com/questions/4648607/stringbuilder-stringbuffer-vs-operator

          For things that compiler will automatically translate from + to StringBuilder, we probably don't have to change. However, the change doesn't really hurt, and because of the formatting done with the patch, readability looks better than original code rather than an issue.

          Some changes made in the patch is used in a loop to construct a string, where StringBuilder is preferred.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Mingliang Liu , Thanks for your question. Please see some discussion here http://stackoverflow.com/questions/4648607/stringbuilder-stringbuffer-vs-operator For things that compiler will automatically translate from + to StringBuilder , we probably don't have to change. However, the change doesn't really hurt, and because of the formatting done with the patch, readability looks better than original code rather than an issue. Some changes made in the patch is used in a loop to construct a string, where StringBuilder is preferred. Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran,

          Rev 004 is actually what you have reviewed and commented earlier, wonder if you have further comment or could you give +1?

          Thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , Rev 004 is actually what you have reviewed and commented earlier, wonder if you have further comment or could you give +1? Thanks a lot.
          Hide
          aw Allen Wittenauer added a comment -

          How does this change interact with dtutil?

          Show
          aw Allen Wittenauer added a comment - How does this change interact with dtutil?
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Allen Wittenauer,

          Thanks for bringing dtutil to my attention, which seems to be a very recent addition to the system. I saw that it does

                 AbstractDelegationTokenIdentifier id =
                      (AbstractDelegationTokenIdentifier) token.decodeIdentifier();
                  out.printf(fmt, token.getKind(), token.getService(),
                             (id != null) ? id.getRenewer() : NA_STRING,
                             (id != null) ? formatDate(id.getMaxDate()) : NA_STRING,
                             token.encodeToUrlString());
          

          that defines its own format and doesn't use the toString() method. So the change I made in HDFS-9732 won't have impact to dtutil unless I missed something.

          Do you agree?

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Allen Wittenauer , Thanks for bringing dtutil to my attention, which seems to be a very recent addition to the system. I saw that it does AbstractDelegationTokenIdentifier id = (AbstractDelegationTokenIdentifier) token.decodeIdentifier(); out.printf(fmt, token.getKind(), token.getService(), (id != null ) ? id.getRenewer() : NA_STRING, (id != null ) ? formatDate(id.getMaxDate()) : NA_STRING, token.encodeToUrlString()); that defines its own format and doesn't use the toString() method. So the change I made in HDFS-9732 won't have impact to dtutil unless I missed something. Do you agree? Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Steve Loughran,

          So far all comments are addressed (Allen Wittenauer, if you disagree, would you please let us know?), would you please see if you could give +1 on rev 004 that you have reviewed?

          Thanks a lot.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Steve Loughran , So far all comments are addressed ( Allen Wittenauer , if you disagree, would you please let us know?), would you please see if you could give +1 on rev 004 that you have reviewed? Thanks a lot.
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for the explanation. String concatenation in loop should use StringBuilder to eliminate the intermediate object construction, which is very true. For other cases in the patch I think the + operator should be as efficient as the StringBuilder. Readability is subjective, and we won't spend time in debating this, though I prefer the + operator for shorter/simpler statement. In last comment, I was wondering other obvious reasons why StringBuilder was explicitly preferred by you guys.

          Other part of the patch looks good to me.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for the explanation. String concatenation in loop should use StringBuilder to eliminate the intermediate object construction, which is very true. For other cases in the patch I think the + operator should be as efficient as the StringBuilder. Readability is subjective, and we won't spend time in debating this, though I prefer the + operator for shorter/simpler statement. In last comment, I was wondering other obvious reasons why StringBuilder was explicitly preferred by you guys. Other part of the patch looks good to me.
          Hide
          stevel@apache.org Steve Loughran added a comment -


          Allen: are you happy with the last discussion? As I'm happy with the patch ... unless you say otherwise I'm going to +1 it later this week

          Show
          stevel@apache.org Steve Loughran added a comment - Allen: are you happy with the last discussion? As I'm happy with the patch ... unless you say otherwise I'm going to +1 it later this week
          Hide
          aw Allen Wittenauer added a comment -

          I'm fine with the changes, I think. Thanks.

          Show
          aw Allen Wittenauer added a comment - I'm fine with the changes, I think. Thanks.
          Hide
          yzhangal Yongjun Zhang added a comment -
          Show
          yzhangal Yongjun Zhang added a comment - Many thanks Steve Loughran and Allen Wittenauer !
          Hide
          yzhangal Yongjun Zhang added a comment -

          HI Steve Loughran, now we have Allen's agreement. Would you please +1? Allen's agreement itself is a +1 but I'd like to see yours official one also thanks.

          Show
          yzhangal Yongjun Zhang added a comment - HI Steve Loughran , now we have Allen's agreement. Would you please +1? Allen's agreement itself is a +1 but I'd like to see yours official one also thanks.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          +1 from me then

          Show
          stevel@apache.org Steve Loughran added a comment - +1 from me then
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I can commit this to trunk; have you tested it in branch-2 yet?

          Show
          stevel@apache.org Steve Loughran added a comment - I can commit this to trunk; have you tested it in branch-2 yet?
          Hide
          cnauroth Chris Nauroth added a comment -

          +1 from me too. Yongjun Zhang, thank you.

          Show
          cnauroth Chris Nauroth added a comment - +1 from me too. Yongjun Zhang , thank you.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thank you all Steve Loughran, Chris Nauroth and Allen Wittenauer. Really appreciate it!

          Thanks for offering to commit Steve, let me try out other branches, and I may just go ahead commit if I don't see issue to save some iteration. I will have to do it later today though.

          Show
          yzhangal Yongjun Zhang added a comment - Thank you all Steve Loughran , Chris Nauroth and Allen Wittenauer . Really appreciate it! Thanks for offering to commit Steve, let me try out other branches, and I may just go ahead commit if I don't see issue to save some iteration. I will have to do it later today though.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'll let you handle it then...

          Show
          stevel@apache.org Steve Loughran added a comment - I'll let you handle it then...
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Steve Loughran, Chris Nauroth and Allen Wittenauer again!

          I just committed to trunk.

          I attempted to cherry-pick to branch-2 etc, and found that there are quite a few conflicts. Then, I found that one dependent jira is incompatible itself and thus not made its path to branch-2, See:

          https://issues.apache.org/jira/browse/HDFS-9085?focusedCommentId=14791503&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14791503

          Even if I get this to branch-2, I found there is still some other missing jiras in branch-2 that makes it unclean. The test I used in TestDelegationTokenFetcher doesn't exist in branch-2.

          At this point, I decide to get it to trunk only, since we are working on 3.0 anyways.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Steve Loughran , Chris Nauroth and Allen Wittenauer again! I just committed to trunk. I attempted to cherry-pick to branch-2 etc, and found that there are quite a few conflicts. Then, I found that one dependent jira is incompatible itself and thus not made its path to branch-2, See: https://issues.apache.org/jira/browse/HDFS-9085?focusedCommentId=14791503&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14791503 Even if I get this to branch-2, I found there is still some other missing jiras in branch-2 that makes it unclean. The test I used in TestDelegationTokenFetcher doesn't exist in branch-2. At this point, I decide to get it to trunk only, since we are working on 3.0 anyways.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #9807 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9807/)
          HDFS-9732, Improve DelegationTokenIdentifier.toString() for better (yzhang: rev e24fe2641b4117601105fa097c8848d82b96b74c)

          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenIdentifier.java
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DelegationTokenFetcher.java
          • hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/security/token/delegation/DelegationTokenIdentifier.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/security/TestDelegationToken.java
          • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #9807 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9807/ ) HDFS-9732 , Improve DelegationTokenIdentifier.toString() for better (yzhang: rev e24fe2641b4117601105fa097c8848d82b96b74c) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenIdentifier.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DelegationTokenFetcher.java hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/security/token/delegation/DelegationTokenIdentifier.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/security/TestDelegationToken.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDelegationTokenFetcher.java

            People

            • Assignee:
              yzhangal Yongjun Zhang
              Reporter:
              stevel@apache.org Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 0.5h
                0.5h
                Remaining:
                Remaining Estimate - 0.5h
                0.5h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development