Giraph
  1. Giraph
  2. GIRAPH-87

Simplify boolean expression in BspService::checkpointFrequencyMet

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

              if (superstep < firstCheckpoint) {
                  return false;
              } else if (((superstep - firstCheckpoint) % checkpointFrequency) == 0) {
                  return true;
              } else {
                  return false;
              }

      can be simplified to just return the result of the else if evaluation.

      1. GIRAPH-87.patch
        0.6 kB
        Eli Reisman
      2. GIRAPH-87.patch
        0.6 kB
        Eli Reisman

        Activity

        Hide
        Arun Suresh added a comment -

        you would still need the first case..
        for eg. if
        superstep = 4
        firstCheckpoint = 6
        checkpointFrequency = 2
        would return true if we only have the else if

        Show
        Arun Suresh added a comment - you would still need the first case.. for eg. if superstep = 4 firstCheckpoint = 6 checkpointFrequency = 2 would return true if we only have the else if
        Hide
        Jakob Homan added a comment -

        correct. This optimization would be to keep the first if statement and then return the result of evaluating the else if argument directly (return (((superstep - ...)

        Show
        Jakob Homan added a comment - correct. This optimization would be to keep the first if statement and then return the result of evaluating the else if argument directly ( return (((superstep - ... )
        Hide
        Eli Reisman added a comment -

        Cleaned up conditional statement w/o changing logic. Just for practice to learn to submit a patch. Ran "mvn test" with Maven3 and it tests successful. My "mvn rat:check" fails because my mojo plugin is not up to date, I will be correcting that now but the patch should be good? Did not test on cluster, btw.

        Thanks for you time, this is my first patch on Apache hope its OK.

        Show
        Eli Reisman added a comment - Cleaned up conditional statement w/o changing logic. Just for practice to learn to submit a patch. Ran "mvn test" with Maven3 and it tests successful. My "mvn rat:check" fails because my mojo plugin is not up to date, I will be correcting that now but the patch should be good? Did not test on cluster, btw. Thanks for you time, this is my first patch on Apache hope its OK.
        Hide
        Eli Reisman added a comment -

        This is the patch for GIRAPH-87 JIRA newbie issue. Passed "mvn test", not tested on cluster.

        Show
        Eli Reisman added a comment - This is the patch for GIRAPH-87 JIRA newbie issue. Passed "mvn test", not tested on cluster.
        Hide
        Jakob Homan added a comment -

        Looks good except it fails checkstyle:

        <file name="/Users/jhoman/repos/giraph/src/main/java/org/apache/giraph/graph/BspService.java">
        <error line="587" severity="error" message="Line matches the illegal pattern &apos;Trailing whitespace&apos;." source="com.puppycrawl.tools.checkstyle.checks.RegexpCheck"/>
        <error line="587" column="5" severity="error" message="&apos;}&apos; should be on the same line." source="com.puppycrawl.tools.checkstyle.checks.blocks.RightCurlyCheck"/>
        <error line="588" severity="error" message="Line matches the illegal pattern &apos;Trailing whitespace&apos;." source="com.puppycrawl.tools.checkstyle.checks.RegexpCheck"/>
        </file>

        Kill the trailing spaces and move the else to the same line and we're good to go.

        Show
        Jakob Homan added a comment - Looks good except it fails checkstyle: <file name="/Users/jhoman/repos/giraph/src/main/java/org/apache/giraph/graph/BspService.java"> <error line="587" severity="error" message="Line matches the illegal pattern &apos;Trailing whitespace&apos;." source="com.puppycrawl.tools.checkstyle.checks.RegexpCheck"/> <error line="587" column="5" severity="error" message="&apos;}&apos; should be on the same line." source="com.puppycrawl.tools.checkstyle.checks.blocks.RightCurlyCheck"/> <error line="588" severity="error" message="Line matches the illegal pattern &apos;Trailing whitespace&apos;." source="com.puppycrawl.tools.checkstyle.checks.RegexpCheck"/> </file> Kill the trailing spaces and move the else to the same line and we're good to go.
        Hide
        Eli Reisman added a comment -

        This is an improved version of GIRAPH-87.patch that passes "mvn checkstyle:check" and of course also "mvn test". Not tested on a cluster.

        Show
        Eli Reisman added a comment - This is an improved version of GIRAPH-87 .patch that passes "mvn checkstyle:check" and of course also "mvn test". Not tested on a cluster.
        Hide
        Avery Ching added a comment -

        +1
        Thanks Eli, I committed on your behalf.

        Show
        Avery Ching added a comment - +1 Thanks Eli, I committed on your behalf.
        Hide
        Hudson added a comment -

        Integrated in Giraph-trunk-Commit #85 (See https://builds.apache.org/job/Giraph-trunk-Commit/85/)
        GIRAPH-87: Simplify boolean expression in
        BspService::checkpointFrequencyMet (Eli Reisman via aching). (Revision 1293545)

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

        • /incubator/giraph/trunk/CHANGELOG
        • /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java
        Show
        Hudson added a comment - Integrated in Giraph-trunk-Commit #85 (See https://builds.apache.org/job/Giraph-trunk-Commit/85/ ) GIRAPH-87 : Simplify boolean expression in BspService::checkpointFrequencyMet (Eli Reisman via aching). (Revision 1293545) Result = SUCCESS aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1293545 Files : /incubator/giraph/trunk/CHANGELOG /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java
        Hide
        Avery Ching added a comment -

        Closing since hudson has commented success.

        Show
        Avery Ching added a comment - Closing since hudson has commented success.
        Hide
        Eli Reisman added a comment -

        Thanks again, looking forward to doing more comprehensive fixes in the
        future!

        On Sat, Feb 25, 2012 at 12:19 AM, Avery Ching (Commented) (JIRA) <

        Show
        Eli Reisman added a comment - Thanks again, looking forward to doing more comprehensive fixes in the future! On Sat, Feb 25, 2012 at 12:19 AM, Avery Ching (Commented) (JIRA) <

          People

          • Assignee:
            Eli Reisman
            Reporter:
            Jakob Homan
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development