Hadoop Common
  1. Hadoop Common
  2. HADOOP-6471

StringBuffer -> StringBuilder - conversion of references as necessary

    Details

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

      Description

      Across hadoop-common codebase, a good number of StringBuffer-s being used are actually candidates for StringBuilders , since the reference does not escape the scope of the method and no concurrency is needed.

      1. HADOOP-6471.patch
        28 kB
        Karthik K
      2. HADOOP-6471.patch
        28 kB
        Karthik K
      3. HADOOP-6471.patch
        31 kB
        Karthik K
      4. HADOOP-6471.patch
        30 kB
        Karthik K

        Activity

        Tom White made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Tom White made changes -
        Fix Version/s 0.21.0 [ 12313563 ]
        Fix Version/s 0.22.0 [ 12314296 ]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk #285 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/285/)
        . StringBuffer -> StringBuilder - conversion of references as necessary. Contributed by Kay Kay.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk #285 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/285/ ) . StringBuffer -> StringBuilder - conversion of references as necessary. Contributed by Kay Kay.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #207 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/207/)
        . StringBuffer -> StringBuilder - conversion of references as necessary. Contributed by Kay Kay.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #207 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/207/ ) . StringBuffer -> StringBuilder - conversion of references as necessary. Contributed by Kay Kay.
        Tom White made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Assignee Kay Kay [ kaykay.unique ]
        Fix Version/s 0.22.0 [ 12314296 ]
        Fix Version/s 0.21.0 [ 12313563 ]
        Resolution Fixed [ 1 ]
        Hide
        Tom White added a comment -

        +1 I've just committed this. Thanks Kay Kay!

        Show
        Tom White added a comment - +1 I've just committed this. Thanks Kay Kay!
        Hide
        Karthik K added a comment -

        This patch seems to pass the test cases. Can somebody with committer access help look into this before the patch becomes out of sync with the trunk.

        Show
        Karthik K added a comment - This patch seems to pass the test cases. Can somebody with committer access help look into this before the patch becomes out of sync with the trunk.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12434582/HADOOP-6471.patch
        against trunk revision 907956.

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

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

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

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

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

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/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/12434582/HADOOP-6471.patch against trunk revision 907956. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/349/console This message is automatically generated.
        Karthik K made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Karthik K made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Karthik K made changes -
        Attachment HADOOP-6471.patch [ 12434582 ]
        Hide
        Karthik K added a comment -

        sync-ing with trunk

        Show
        Karthik K added a comment - sync-ing with trunk
        Hide
        Karthik K added a comment -

        can somebody help review this ..

        Show
        Karthik K added a comment - can somebody help review this ..
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429959/HADOOP-6471.patch
        against trunk revision 897023.

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

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

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

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

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

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/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/12429959/HADOOP-6471.patch against trunk revision 897023. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/271/console This message is automatically generated.
        Karthik K made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Karthik K made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Karthik K made changes -
        Attachment HADOOP-6471.patch [ 12429959 ]
        Hide
        Karthik K added a comment -

        Methods deprecated:

        Environment.runCommand(String)
        Enrivonment.runCommand(String[])
        LocalStore.pack(SerializedRecord)

        Methods added:

        Environment.runCommandGeneric(String)
        Environment.runCommandGeneric(String[])
        LocalStore.packConcurrent(SerializedRecord)

        $ grep '^-' HADOOP-6471.patch | grep public
        returns no entries.

        Show
        Karthik K added a comment - Methods deprecated: Environment.runCommand(String) Enrivonment.runCommand(String[]) LocalStore.pack(SerializedRecord) Methods added: Environment.runCommandGeneric(String) Environment.runCommandGeneric(String[]) LocalStore.packConcurrent(SerializedRecord) $ grep '^-' HADOOP-6471 .patch | grep public returns no entries.
        Hide
        Karthik K added a comment -

        Changing the return type of a public method is still an incompatible change, however. The old method needs to be deprecated first, before removing it. The new method would have to have a different name since methods can't be overloaded on return type alone.

        Understandable. Will post a revised one with the deprecated marker as well.

        Show
        Karthik K added a comment - Changing the return type of a public method is still an incompatible change, however. The old method needs to be deprecated first, before removing it. The new method would have to have a different name since methods can't be overloaded on return type alone. Understandable. Will post a revised one with the deprecated marker as well.
        Hide
        Tom White added a comment -

        Changing the return type of a public method is still an incompatible change, however. The old method needs to be deprecated first, before removing it. The new method would have to have a different name since methods can't be overloaded on return type alone.

        Show
        Tom White added a comment - Changing the return type of a public method is still an incompatible change, however. The old method needs to be deprecated first, before removing it. The new method would have to have a different name since methods can't be overloaded on return type alone.
        Hide
        Karthik K added a comment -

        Actually CharSequence fits the syntax better. Revised patch attached.

        Show
        Karthik K added a comment - Actually CharSequence fits the syntax better. Revised patch attached.
        Hide
        steve_l added a comment -

        that would be OK to me, and this is a contrib/ so it's less critical. What do others think?

        Show
        steve_l added a comment - that would be OK to me, and this is a contrib/ so it's less critical. What do others think?
        Karthik K made changes -
        Attachment HADOOP-6471.patch [ 12429615 ]
        Hide
        Karthik K added a comment -
        I'm +1 to going s/StringBuffer/r/StringBuilder/ inside methods, but this patch also changes some of the public methods in classes in contrib/failmon, which could have compatibility issues. Better to omit those, or, if there is a pressing need to change their signature, switch them to returning a string or a char sequence, either of which will hide the details of how the string is built up

        Those changes were because they were used by one of the internal methods using StringBuffer - so they go in with this change. Agree about loosely coupled type in the signature though. Woud java.lang.Appendable be ok ?

        Show
        Karthik K added a comment - I'm +1 to going s/StringBuffer/r/StringBuilder/ inside methods, but this patch also changes some of the public methods in classes in contrib/failmon, which could have compatibility issues. Better to omit those, or, if there is a pressing need to change their signature, switch them to returning a string or a char sequence, either of which will hide the details of how the string is built up Those changes were because they were used by one of the internal methods using StringBuffer - so they go in with this change. Agree about loosely coupled type in the signature though. Woud java.lang.Appendable be ok ?
        Hide
        steve_l added a comment -

        I'm +1 to going s/StringBuffer/r/StringBuilder/ inside methods, but this patch also changes some of the public methods in classes in contrib/failmon, which could have compatibility issues. Better to omit those, or, if there is a pressing need to change their signature, switch them to returning a string or a char sequence, either of which will hide the details of how the string is built up

        Show
        steve_l added a comment - I'm +1 to going s/StringBuffer/r/StringBuilder/ inside methods, but this patch also changes some of the public methods in classes in contrib/failmon, which could have compatibility issues. Better to omit those, or, if there is a pressing need to change their signature, switch them to returning a string or a char sequence, either of which will hide the details of how the string is built up
        Karthik K made changes -
        Fix Version/s 0.21.0 [ 12313563 ]
        Fix Version/s 0.20.2 [ 12314203 ]
        Hide
        Karthik K added a comment -

        Moving to higher revision as it is not a bug fix.

        Show
        Karthik K added a comment - Moving to higher revision as it is not a bug fix.
        Karthik K made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Karthik K made changes -
        Summary StringBuilder -> StringBuilder unnecessary references StringBuffer -> StringBuilder - conversion of references as necessary
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429003/HADOOP-6471.patch
        against trunk revision 893666.

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

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

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

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

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

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/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/12429003/HADOOP-6471.patch against trunk revision 893666. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/242/console This message is automatically generated.
        Karthik K made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Karthik K made changes -
        Field Original Value New Value
        Attachment HADOOP-6471.patch [ 12429003 ]
        Karthik K created issue -

          People

          • Assignee:
            Karthik K
            Reporter:
            Karthik K
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development