Mahout
  1. Mahout
  2. MAHOUT-1187

Upgrade Commons Lang to Commons Lang3.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.8
    • Fix Version/s: 0.8
    • Component/s: None
    • Labels:
      None

      Description

      Upgrading Mahout codebase to using Commons Lang3.

      1. Mahout-1187.patch
        23 kB
        Suneel Marthi

        Activity

        Hide
        Sean Owen added a comment -

        It's a good idea but IIRC it is not directly used but needed by a dependency Might investigate and double-check that. It may no longer be true. And we shouldn't have to declare it if it's only needed by a dependency. and this project can use .lang3. alongside .lang. anyway.

        Show
        Sean Owen added a comment - It's a good idea but IIRC it is not directly used but needed by a dependency Might investigate and double-check that. It may no longer be true. And we shouldn't have to declare it if it's only needed by a dependency. and this project can use .lang3. alongside .lang. anyway.
        Hide
        Suneel Marthi added a comment -

        I see that its not merely a dependency, but is actually used in the code and Test Cases. See ConfusionMatrix.java, VectorBenchmarks.java (for eg.) and a few other places. I'll be uploading a patch today and send it your way for review (and commit to trunk). Not sure if I have commit access to SVN yet.

        Show
        Suneel Marthi added a comment - I see that its not merely a dependency, but is actually used in the code and Test Cases. See ConfusionMatrix.java, VectorBenchmarks.java (for eg.) and a few other places. I'll be uploading a patch today and send it your way for review (and commit to trunk). Not sure if I have commit access to SVN yet.
        Hide
        Suneel Marthi added a comment -

        Patch available

        Show
        Suneel Marthi added a comment - Patch available
        Hide
        Suneel Marthi added a comment -

        Sean, for your review.

        Show
        Suneel Marthi added a comment - Sean, for your review.
        Hide
        Sean Owen added a comment - - edited

        Looks like a straightforward mechanical change. I'd just remove the blank "@throws Exception" that got added to a javadoc statement.

        PS I'll let you do the honors of committing as and when you have access, so as not to take away the fun. But I can commit if you need me to.

        Show
        Sean Owen added a comment - - edited Looks like a straightforward mechanical change. I'd just remove the blank "@throws Exception" that got added to a javadoc statement. PS I'll let you do the honors of committing as and when you have access, so as not to take away the fun. But I can commit if you need me to.
        Hide
        Suneel Marthi added a comment -

        It was straightforward, been through this exercise before at work so knew clearly as to what it entails.
        I'll let you commit this for me. Someone needs to walk me through this commit process, I have my Apache Id created.

        Show
        Suneel Marthi added a comment - It was straightforward, been through this exercise before at work so knew clearly as to what it entails. I'll let you commit this for me. Someone needs to walk me through this commit process, I have my Apache Id created.
        Hide
        Suneel Marthi added a comment -

        Uploading new patch based on Sean's feedback.

        Show
        Suneel Marthi added a comment - Uploading new patch based on Sean's feedback.
        Hide
        Sean Owen added a comment -

        It should be as simple as "svn commit" and logging in with your new Apache ID / password. Go ahead and try it to test. I think you have to have checked out the code from the HTTPS URL, note.

        Show
        Sean Owen added a comment - It should be as simple as "svn commit" and logging in with your new Apache ID / password. Go ahead and try it to test. I think you have to have checked out the code from the HTTPS URL, note.
        Hide
        Suneel Marthi added a comment -

        Thanks Sean for the pointer, will do it later today. Assigning this to myself for now and I'll do the honors of committing to SVN. I first need to checkout the codebase from HTTPS URL.

        Show
        Suneel Marthi added a comment - Thanks Sean for the pointer, will do it later today. Assigning this to myself for now and I'll do the honors of committing to SVN. I first need to checkout the codebase from HTTPS URL.
        Hide
        Suneel Marthi added a comment -

        Committed Changes to trunk.

        Show
        Suneel Marthi added a comment - Committed Changes to trunk.
        Hide
        Hudson added a comment -

        Integrated in Mahout-Quality #1953 (See https://builds.apache.org/job/Mahout-Quality/1953/)
        Mahout-1187: Update Commons Lang to Commons Lang3 (Revision 1465425)

        Result = SUCCESS

        Show
        Hudson added a comment - Integrated in Mahout-Quality #1953 (See https://builds.apache.org/job/Mahout-Quality/1953/ ) Mahout-1187: Update Commons Lang to Commons Lang3 (Revision 1465425) Result = SUCCESS
        Hide
        Hudson added a comment -

        Integrated in Mahout-Quality #1956 (See https://builds.apache.org/job/Mahout-Quality/1956/)
        MAHOUT-1187: Update Commons Lang to Commons Lang3, added missing changelog entry (Revision 1465886)

        Result = SUCCESS
        ssc :
        Files :

        • /mahout/trunk/CHANGELOG
        Show
        Hudson added a comment - Integrated in Mahout-Quality #1956 (See https://builds.apache.org/job/Mahout-Quality/1956/ ) MAHOUT-1187 : Update Commons Lang to Commons Lang3, added missing changelog entry (Revision 1465886) Result = SUCCESS ssc : Files : /mahout/trunk/CHANGELOG
        Hide
        Ted Dunning added a comment -

        One parting shot here.

        I just took a look and it appears that there are three uses for lang3 in Mahout

        a) as a way of getting atomic primitive types. I think AtomicBoolean and friends are better here.

        b) in a massive append/StringUtils.rightPad expression that should be replaced with a String.printf

        c) in ArrayUtil expressions that could likely be replaced by either guava or standard Java.

        So actually dropping the dependency is viable.

        Show
        Ted Dunning added a comment - One parting shot here. I just took a look and it appears that there are three uses for lang3 in Mahout a) as a way of getting atomic primitive types. I think AtomicBoolean and friends are better here. b) in a massive append/StringUtils.rightPad expression that should be replaced with a String.printf c) in ArrayUtil expressions that could likely be replaced by either guava or standard Java. So actually dropping the dependency is viable.
        Hide
        Suneel Marthi added a comment -

        Agreeing with Ted on all 3 points. Lang3 can definitely be dispensed with and replaced by Java/Guava. Let me create another Jira and work on this.

        Show
        Suneel Marthi added a comment - Agreeing with Ted on all 3 points. Lang3 can definitely be dispensed with and replaced by Java/Guava. Let me create another Jira and work on this.
        Hide
        Suneel Marthi added a comment -

        Ted, while we are at it, it also makes sense to upgrade Guava to Guava 15.x (from present 14.x). I see a lot of deprecated calls to 'Closeables.closeQuetly()' and was almost tempted to fix those as part of this JIRA. Thoughts?

        Show
        Suneel Marthi added a comment - Ted, while we are at it, it also makes sense to upgrade Guava to Guava 15.x (from present 14.x). I see a lot of deprecated calls to 'Closeables.closeQuetly()' and was almost tempted to fix those as part of this JIRA. Thoughts?
        Hide
        Sean Owen added a comment -

        Guava 14 is the latest release and it has the replacement method. I support changing these to "Closeables.close(closeable, true);" FWIW

        Show
        Sean Owen added a comment - Guava 14 is the latest release and it has the replacement method. I support changing these to "Closeables.close(closeable, true);" FWIW

          People

          • Assignee:
            Suneel Marthi
            Reporter:
            Suneel Marthi
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development