Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3014

FSEditLogOp and its subclasses should have toString() method

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.0-alpha
    • Component/s: namenode
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      FSEditLogOp and its subclasses should have toString() method.
      It's helpful for investigating editlog related issues.

      1. HDFS-3014.txt
        18 kB
        Sho Shimauchi
      2. HDFS-3014.txt
        18 kB
        Sho Shimauchi

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1860 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1860/)
          Moving HDFS-3036, HDFS-3014, HDFS-3003, HDFS-2878, HDFS-208, HDFS-2764, HDFS-2410, HDFS-2285, HDFS-2507 to 0.23.3 section (Revision 1298274)

          Result = ABORTED
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298274
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1860 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1860/ ) Moving HDFS-3036 , HDFS-3014 , HDFS-3003 , HDFS-2878 , HDFS-208 , HDFS-2764 , HDFS-2410 , HDFS-2285 , HDFS-2507 to 0.23.3 section (Revision 1298274) Result = ABORTED suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298274 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1926 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1926/)
          Moving HDFS-3036, HDFS-3014, HDFS-3003, HDFS-2878, HDFS-208, HDFS-2764, HDFS-2410, HDFS-2285, HDFS-2507 to 0.23.3 section (Revision 1298274)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298274
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1926 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1926/ ) Moving HDFS-3036 , HDFS-3014 , HDFS-3003 , HDFS-2878 , HDFS-208 , HDFS-2764 , HDFS-2410 , HDFS-2285 , HDFS-2507 to 0.23.3 section (Revision 1298274) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298274 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Commit #669 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/669/)
          HDFS-3014. Merge r1294926 from trunk to 0.23 (Revision 1298244)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298244
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Commit #669 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/669/ ) HDFS-3014 . Merge r1294926 from trunk to 0.23 (Revision 1298244) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298244 Files : /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Sho Shimauchi added a comment -

          thanks!

          Show
          Sho Shimauchi added a comment - thanks!
          Hide
          Aaron T. Myers added a comment -

          Yep, let's mark this resolved. I've spent the day looking for findbugs and other warnings and such on trunk and other branches, and nothing came up because of this patch.

          Show
          Aaron T. Myers added a comment - Yep, let's mark this resolved. I've spent the day looking for findbugs and other warnings and such on trunk and other branches, and nothing came up because of this patch.
          Hide
          Sho Shimauchi added a comment -

          @atm:
          Thanks for commiting the patch.
          Then can this JIRA be marked as solved?

          Show
          Sho Shimauchi added a comment - @atm: Thanks for commiting the patch. Then can this JIRA be marked as solved?
          Hide
          Aaron T. Myers added a comment -

          If you have checked, then it is fine. Please remember to wait for Jenkins next time.

          Yep, sorry. Just completely slipped my mind.

          Show
          Aaron T. Myers added a comment - If you have checked, then it is fine. Please remember to wait for Jenkins next time. Yep, sorry. Just completely slipped my mind.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > Nicholas, do you want me to revert this patch so Jenkins can run? I feel confident from manual inspection that everything is fine.

          If you have checked, then it is fine. Please remember to wait for Jenkins next time.

          Warnings are much more easy to be sneaked in without Jenkins. For example, there are 43 warnings in TestAuthenticationFilter which were committed as a part of Alfredo without Jenkins. See HADOOP-8119.

          Show
          Tsz Wo Nicholas Sze added a comment - > Nicholas, do you want me to revert this patch so Jenkins can run? I feel confident from manual inspection that everything is fine. If you have checked, then it is fine. Please remember to wait for Jenkins next time. Warnings are much more easy to be sneaked in without Jenkins. For example, there are 43 warnings in TestAuthenticationFilter which were committed as a part of Alfredo without Jenkins. See HADOOP-8119 .
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1005 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1005/)
          HDFS-3014. FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1005 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1005/ ) HDFS-3014 . FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #970 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/970/)
          HDFS-3014. FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #970 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/970/ ) HDFS-3014 . FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Aaron T. Myers added a comment -

          Sigh. Of course the patch can't apply now - it's already in trunk.

          Nicholas, do you want me to revert this patch so Jenkins can run? I feel confident from manual inspection that everything is fine.

          Show
          Aaron T. Myers added a comment - Sigh. Of course the patch can't apply now - it's already in trunk. Nicholas, do you want me to revert this patch so Jenkins can run? I feel confident from manual inspection that everything is fine.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12516341/HDFS-3014.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1927//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12516341/HDFS-3014.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1927//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          Why there is no Jenkins report before committing?

          Whoops! Sorry, I thought I had marked it patch available, but clearly I forgot to.

          Let me do that now. It will probably come back clean, but if it doesn't, I'll revert it.

          My apologies.

          Show
          Aaron T. Myers added a comment - Why there is no Jenkins report before committing? Whoops! Sorry, I thought I had marked it patch available, but clearly I forgot to. Let me do that now. It will probably come back clean, but if it doesn't, I'll revert it. My apologies.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1809 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1809/)
          HDFS-3014. FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926)

          Result = ABORTED
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1809 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1809/ ) HDFS-3014 . FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926) Result = ABORTED atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Why there is no Jenkins report before committing?

          Show
          Tsz Wo Nicholas Sze added a comment - Why there is no Jenkins report before committing?
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1801 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1801/)
          HDFS-3014. FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1801 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1801/ ) HDFS-3014 . FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1875 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1875/)
          HDFS-3014. FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926)

          Result = SUCCESS
          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1875 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1875/ ) HDFS-3014 . FSEditLogOp and its subclasses should have toString() method. Contributed by Sho Shimauchi. (Revision 1294926) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1294926 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
          Hide
          Aaron T. Myers added a comment -

          I've just committed this to trunk. Thanks a lot, Sho!

          Show
          Aaron T. Myers added a comment - I've just committed this to trunk. Thanks a lot, Sho!
          Hide
          Sho Shimauchi added a comment -

          sure, I fixed it.

          Show
          Sho Shimauchi added a comment - sure, I fixed it.
          Hide
          Aaron T. Myers added a comment -

          OK then, fine by me.

          One other comment then: I think we should refactor the toString methods of AddOp and CloseOp, since they're almost identical and those classes have a common superclass. Perhaps AddCloseOp could have a "stringifyMembers()" method which the subclasses could call?

          +1 once this is addressed.

          Show
          Aaron T. Myers added a comment - OK then, fine by me. One other comment then: I think we should refactor the toString methods of AddOp and CloseOp, since they're almost identical and those classes have a common superclass. Perhaps AddCloseOp could have a "stringifyMembers()" method which the subclasses could call? +1 once this is addressed.
          Hide
          Sho Shimauchi added a comment -

          The reason why I prefer to use StringBuilder is that it's faster than String.format().
          These toString() will be called only when developer debugs fsimage/edits related issues.
          This method won't be called not so often, therefore most user will never see these outputs.

          However, once this method is called, it's probably called many times.
          For example, I'll use this method to print debug log of replaying edit log (see HDFS-309 ).
          Using StringBuilder will reduce time to replay edit log.
          That means developer will reduce time to diagnose corrupted edit log and therefore the debeloper can seek the root cause of the issue faster.

          Notes that I don't have strong opinion about this either.
          I'll change the patch if it's better.

          Show
          Sho Shimauchi added a comment - The reason why I prefer to use StringBuilder is that it's faster than String.format(). These toString() will be called only when developer debugs fsimage/edits related issues. This method won't be called not so often, therefore most user will never see these outputs. However, once this method is called, it's probably called many times. For example, I'll use this method to print debug log of replaying edit log (see HDFS-309 ). Using StringBuilder will reduce time to replay edit log. That means developer will reduce time to diagnose corrupted edit log and therefore the debeloper can seek the root cause of the issue faster. Notes that I don't have strong opinion about this either. I'll change the patch if it's better.
          Hide
          Aaron T. Myers added a comment -

          It's mostly personal preference. I just find this:

          builder.append("AddOp [length=");
          builder.append(length);
          builder.append(", path=");
          builder.append(path);
          builder.append(", replication=");
          builder.append(replication);
          builder.append(", mtime=");
          builder.append(mtime);
          builder.append(", atime=");
          builder.append(atime);
          builder.append(", blockSize=");
          builder.append(blockSize);
          builder.append(", blocks=");
          builder.append(Arrays.toString(blocks));
          builder.append(", permissions=");
          builder.append(permissions);
          builder.append(", clientName=");
          builder.append(clientName);
          builder.append(", clientMachine=");
          builder.append(clientMachine);
          builder.append(", opCode=");
          builder.append(opCode);
          builder.append(", txid=");
          builder.append(txid);
          builder.append("]");
          

          to be less readable than this:

          String.format("AddOp [length=%d, path=%s, replication=%d, mtime=%d, atime=%d, blocksize=%d, " +
          "blocks=%s, permissions=%s, clientName=%s, clientMachine=%s, opCode=%d, txid=%d]",
          length, path, replication, mtime, atime, blockSize, Arrays.toString(blocks), permissions,
          clientName, clientMachine, opCode, txid);
          

          I also suspect that if one were to do a detailed analysis of StringBuilder vs. String.format usage in the code base, you'd probably find that StringBuilder is used mostly when there is an open-ended number of items to be included in the string (e.g. when stringifying a list of indeterminate length), and therefore String.format couldn't possibly be used.

          Regardless, I don't feel very strongly about this point. If you think it's better to use StringBuilder, then I don't mind committing it as you've implemented it.

          Thoughts?

          Show
          Aaron T. Myers added a comment - It's mostly personal preference. I just find this: builder.append( "AddOp [length=" ); builder.append(length); builder.append( ", path=" ); builder.append(path); builder.append( ", replication=" ); builder.append(replication); builder.append( ", mtime=" ); builder.append(mtime); builder.append( ", atime=" ); builder.append(atime); builder.append( ", blockSize=" ); builder.append(blockSize); builder.append( ", blocks=" ); builder.append(Arrays.toString(blocks)); builder.append( ", permissions=" ); builder.append(permissions); builder.append( ", clientName=" ); builder.append(clientName); builder.append( ", clientMachine=" ); builder.append(clientMachine); builder.append( ", opCode=" ); builder.append(opCode); builder.append( ", txid=" ); builder.append(txid); builder.append( "]" ); to be less readable than this: String .format( "AddOp [length=%d, path=%s, replication=%d, mtime=%d, atime=%d, blocksize=%d, " + "blocks=%s, permissions=%s, clientName=%s, clientMachine=%s, opCode=%d, txid=%d]" , length, path, replication, mtime, atime, blockSize, Arrays.toString(blocks), permissions, clientName, clientMachine, opCode, txid); I also suspect that if one were to do a detailed analysis of StringBuilder vs. String.format usage in the code base, you'd probably find that StringBuilder is used mostly when there is an open-ended number of items to be included in the string (e.g. when stringifying a list of indeterminate length), and therefore String.format couldn't possibly be used. Regardless, I don't feel very strongly about this point. If you think it's better to use StringBuilder, then I don't mind committing it as you've implemented it. Thoughts?
          Hide
          Sho Shimauchi added a comment -

          Can you tell me why String.format() is better than StringBuilder in this case?
          StringBuilder is better than String.format() in performance I think.
          Additionally, StringBuilder is used 456 times in trunk while String.format() is used 242 times. That mean StringBuilder seems to be more common in code.
          I don't think StringBuilder + append() is less readable than String.format(), but if you feel so, I'll change the format.

          Show
          Sho Shimauchi added a comment - Can you tell me why String.format() is better than StringBuilder in this case? StringBuilder is better than String.format() in performance I think. Additionally, StringBuilder is used 456 times in trunk while String.format() is used 242 times. That mean StringBuilder seems to be more common in code. I don't think StringBuilder + append() is less readable than String.format(), but if you feel so, I'll change the format.
          Hide
          Aaron T. Myers added a comment -

          Hey Sho, instead of using a StringBuilder and repeatedly appending, have you considered using String.format(...)? I think that might be a little bit clearer.

          Show
          Aaron T. Myers added a comment - Hey Sho, instead of using a StringBuilder and repeatedly appending, have you considered using String.format(...)? I think that might be a little bit clearer.
          Hide
          Sho Shimauchi added a comment -

          attached

          Show
          Sho Shimauchi added a comment - attached

            People

            • Assignee:
              Sho Shimauchi
              Reporter:
              Sho Shimauchi
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development