Mahout
  1. Mahout
  2. MAHOUT-480

Replace manual precondition checking with Precondition utility class from Guava

    Details

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

      Description

      Replace manual precondition checking with the Precondition class. This will affect the following classes:
      FileDataModel
      MemoryDiffStorage
      SlopeOneRecommender
      FileDiffStorage
      AbstractJDBCDiffStorage

      1. MAHOUT-480.patch
        152 kB
        Ted Dunning
      2. MAHOUT-480_v3.patch
        140 kB
        Eugen Paraschiv

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Mahout-Quality #368 (See https://hudson.apache.org/hudson/job/Mahout-Quality/368/ ) MAHOUT-480
        Hide
        Ted Dunning added a comment -

        Thanks Sean.

        I have the same preference as you do. Argument checking is argument checking and null pointer checking in the code is null pointer checking. The distinction is (slightly) nice to preserve. On the other hand, I would prioritize ease of reading of the exception type, but only if somebody showed me
        code that was significantly easier to read.

        Show
        Ted Dunning added a comment - Thanks Sean. I have the same preference as you do. Argument checking is argument checking and null pointer checking in the code is null pointer checking. The distinction is (slightly) nice to preserve. On the other hand, I would prioritize ease of reading of the exception type, but only if somebody showed me code that was significantly easier to read.
        Hide
        Sean Owen added a comment -

        Ted I have the patch locally and have added to it as well as fixed a few more typos. I can commit shortly.

        This is a very small point of preference, not one I would insist on, but I like IllegalArgumentException for when you've checked an argument for null and found it to be null, and it can't be. It is perhaps more explicit – the problem is a bad argument. The description can say what is null. NPE always causes me to guess that there was a developer error inside the library at first glance.

        Show
        Sean Owen added a comment - Ted I have the patch locally and have added to it as well as fixed a few more typos. I can commit shortly. This is a very small point of preference, not one I would insist on, but I like IllegalArgumentException for when you've checked an argument for null and found it to be null, and it can't be. It is perhaps more explicit – the problem is a bad argument. The description can say what is null. NPE always causes me to guess that there was a developer error inside the library at first glance.
        Hide
        Ted Dunning added a comment -

        I agree that leaving out math for now is a good idea.

        Has anybody finished checking my patch?

        SHould I commit it anyway?

        Eugen,

        I don't think it is so bad to change the type of exception if it still makes sense. Thus, changing an IllegalArgumentException to a NullPointerException isn't such a bad thing if it makes the code easier to read. It is officially an API change, but a fairly benign one.

        Show
        Ted Dunning added a comment - I agree that leaving out math for now is a good idea. Has anybody finished checking my patch? SHould I commit it anyway? Eugen, I don't think it is so bad to change the type of exception if it still makes sense. Thus, changing an IllegalArgumentException to a NullPointerException isn't such a bad thing if it makes the code easier to read. It is officially an API change, but a fairly benign one.
        Hide
        Sean Owen added a comment -

        No that's fine, I will do the rest, to match your effort. I don't think we should do mahout-math – I believe we'd wanted to keep the dependencies there minimal as it is reused by at least one other project. (Well, that was my argument at least.)

        Show
        Sean Owen added a comment - No that's fine, I will do the rest, to match your effort. I don't think we should do mahout-math – I believe we'd wanted to keep the dependencies there minimal as it is reused by at least one other project. (Well, that was my argument at least.)
        Hide
        Eugen Paraschiv added a comment -

        I fully understand your concern about inconsistent state, especially from a conceptual point of view. I will open JIRA issues for mahout-math, mahout-examples and mahout-utils and will provide similar patches for them if that is OK.
        From the API perspective, this kind of cross cutting concern is less of a problem, as the guava checks are perfectly aligned with the manual checks that are in place now, meaning that the type of exception thrown is the same, so incremental changes are less of a problem.
        So, now that mahout-core is covered, please tell me how I should go about creating the other JIRA issues so that I can continue with this.
        Thanks.

        Show
        Eugen Paraschiv added a comment - I fully understand your concern about inconsistent state, especially from a conceptual point of view. I will open JIRA issues for mahout-math, mahout-examples and mahout-utils and will provide similar patches for them if that is OK. From the API perspective, this kind of cross cutting concern is less of a problem, as the guava checks are perfectly aligned with the manual checks that are in place now, meaning that the type of exception thrown is the same, so incremental changes are less of a problem. So, now that mahout-core is covered, please tell me how I should go about creating the other JIRA issues so that I can continue with this. Thanks.
        Hide
        Sean Owen added a comment -

        I think this is a fairly heroic effort to use Preconditions across the board. My concern had been about getting into a half-way state where Preconditions was used for some but not all such checking. It's the same sort of issue we'd discussed any number of times about, say, half-using 3 logging systems is worse than none at all, or same for 3 math libraries.

        I find this less of an issue here – not all arg checking can or must be done with Preconditions. We have no existing consistent system to compete with.

        And, it looks largely like mahout-core is updated. mahout-examples and mahout-utils are not it seems. I could take that on as well as try to find more instances where it can be used in mahout-core.

        mahout-math, perhaps we should not introduce the dependency into.

        Show
        Sean Owen added a comment - I think this is a fairly heroic effort to use Preconditions across the board. My concern had been about getting into a half-way state where Preconditions was used for some but not all such checking. It's the same sort of issue we'd discussed any number of times about, say, half-using 3 logging systems is worse than none at all, or same for 3 math libraries. I find this less of an issue here – not all arg checking can or must be done with Preconditions. We have no existing consistent system to compete with. And, it looks largely like mahout-core is updated. mahout-examples and mahout-utils are not it seems. I could take that on as well as try to find more instances where it can be used in mahout-core. mahout-math, perhaps we should not introduce the dependency into.
        Hide
        Ted Dunning added a comment -

        I think that this can go in if we get one more pair of eyes on my latest patch.

        It may need -p1 to apply. Or -p0. I am using git through IDEA which makes it hard to say which kind of patch is produced.

        Show
        Ted Dunning added a comment - I think that this can go in if we get one more pair of eyes on my latest patch. It may need -p1 to apply. Or -p0. I am using git through IDEA which makes it hard to say which kind of patch is produced.
        Hide
        Ted Dunning added a comment -

        Eugen,

        I think that there were half a dozen errors in your conversions and at least one error in the original code. Your patch also had some problems being applied on 3 files, probably due to recent commits.

        I updated the patch to apply cleanly and I have reviewed all 93 files that you changed, correcting the following kinds of items:

        • some tests were not negated correctly
        • some of the original checks were backwards
        • when data is included in the message, printf syntax should be used to avoid a string construction if the precondition holds
        • code should use Mahout style

        Can you review this revised patch?

        Also, it is good to keep the same name on the file so that JIRA can see earlier versions and deprecate them. This can be very important on projects like Hadoop where patches are reviewed automatically.

        Show
        Ted Dunning added a comment - Eugen, I think that there were half a dozen errors in your conversions and at least one error in the original code. Your patch also had some problems being applied on 3 files, probably due to recent commits. I updated the patch to apply cleanly and I have reviewed all 93 files that you changed, correcting the following kinds of items: some tests were not negated correctly some of the original checks were backwards when data is included in the message, printf syntax should be used to avoid a string construction if the precondition holds code should use Mahout style Can you review this revised patch? Also, it is good to keep the same name on the file so that JIRA can see earlier versions and deprecate them. This can be very important on projects like Hadoop where patches are reviewed automatically.
        Hide
        Eugen Paraschiv added a comment -

        The MAHOUT-480_v3.patch contains fixes for all of the mahout-core condition checking that was previously done by hand. A few notes:

        • some of the checks are verifying multiple independent conditions on one single line; I left most of those as they were, but they should really be broken into independent checks - I can do that after this patch is integrated
        • the guava Preconditions class has more specialized methods for checking various types of conditions; I only used Preconditions.checkArgument() so that the exact type of exception thrown doesn't change (for example some of the other methods throw NullPointerException instead of IllegalArgumentException)
        Show
        Eugen Paraschiv added a comment - The MAHOUT-480 _v3.patch contains fixes for all of the mahout-core condition checking that was previously done by hand. A few notes: some of the checks are verifying multiple independent conditions on one single line; I left most of those as they were, but they should really be broken into independent checks - I can do that after this patch is integrated the guava Preconditions class has more specialized methods for checking various types of conditions; I only used Preconditions.checkArgument() so that the exact type of exception thrown doesn't change (for example some of the other methods throw NullPointerException instead of IllegalArgumentException)
        Hide
        Eugen Paraschiv added a comment -

        Sure, I will submit a full patch in a few days. Thanks.

        Show
        Eugen Paraschiv added a comment - Sure, I will submit a full patch in a few days. Thanks.
        Hide
        Ted Dunning added a comment -

        Eugen,

        This seems like something that can go into 0.4 if you would like to champion the issue.

        Show
        Ted Dunning added a comment - Eugen, This seems like something that can go into 0.4 if you would like to champion the issue.
        Hide
        Sean Owen added a comment -

        Looking good, are you going further with it?

        Show
        Sean Owen added a comment - Looking good, are you going further with it?
        Hide
        Eugen Paraschiv added a comment - - edited

        This patch covers a much wider portion of the system, in accordance with the feedback. The goal is to replace manual condition checking throwing IllegalArgumentException with Preconditions.checkArguments calls. If the feedback on this is OK, I will of course continue to cover the rest of the system and provide a second patch.

        Show
        Eugen Paraschiv added a comment - - edited This patch covers a much wider portion of the system, in accordance with the feedback. The goal is to replace manual condition checking throwing IllegalArgumentException with Preconditions.checkArguments calls. If the feedback on this is OK, I will of course continue to cover the rest of the system and provide a second patch.
        Hide
        Sean Owen added a comment -

        Good one, though the thrust of my message on the dev@ thread was that we ought to do argument checking across the entire codebase this way, or not. I don't want to use Guava in 5 classes only.

        But I am at the least happy to make sure that the behavior of the code matches what's in this patch.

        Show
        Sean Owen added a comment - Good one, though the thrust of my message on the dev@ thread was that we ought to do argument checking across the entire codebase this way, or not. I don't want to use Guava in 5 classes only. But I am at the least happy to make sure that the behavior of the code matches what's in this patch.
        Hide
        Eugen Paraschiv added a comment -

        This patch contains initial work on this issue - it does not modify the exception logic of the code at all (throws the exact same exception type with the same message). There are still a few instances where the code might benefit from additional checks.

        Show
        Eugen Paraschiv added a comment - This patch contains initial work on this issue - it does not modify the exception logic of the code at all (throws the exact same exception type with the same message). There are still a few instances where the code might benefit from additional checks.

          People

          • Assignee:
            Sean Owen
            Reporter:
            Eugen Paraschiv
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development