Giraph
  1. Giraph
  2. GIRAPH-112

Use elements() properly in LongDoubleFloatDoubleVertex

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.1.0
    • Fix Version/s: 0.1.0
    • Component/s: graph
    • Labels:
      None
    • Environment:

      Any

      Description

      I found a bug in LongDoubleFloatDoubleVertex.write(DataOutput out) when running a small graph algorithm. The symptom is that a vertex read from a different worker becomes junk after the RPC communication. And the source of the problem is the writing of the messages in LongDoubleFloatDoubleVertex.write(DataOutput out):

      for(double msg : messageList.elements())

      { out.writeDouble(msg); }

      Here messageList.elements() will returns all the elements currently stored in the mahout DoubleArrayList, even including invalid elements between size and capacity. Therefore, the write() function will write a bunch of invalid messages, which will cause error when reading them back in readfields().

      The following is a simple solution:

      double[] elements=messageList.elements();
      for(int i=0; i<messageList.size(); i++)

      { out.writeDouble(elements[i]); }

        Activity

        Hide
        Avery Ching added a comment -

        Actually, elements() is used more than once. This patch should address all of them.

        Show
        Avery Ching added a comment - Actually, elements() is used more than once. This patch should address all of them.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3287/
        -----------------------------------------------------------

        Review request for giraph.

        Summary
        -------

        As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity.

        Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases.

        Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java.

        This addresses bug GIRAPH-112.
        https://issues.apache.org/jira/browse/GIRAPH-112

        Diffs


        http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634

        Diff: https://reviews.apache.org/r/3287/diff

        Testing
        -------

        Local unittests and MR unittests.

        Thanks,

        Avery

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3287/ ----------------------------------------------------------- Review request for giraph. Summary ------- As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity. Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases. Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java. This addresses bug GIRAPH-112 . https://issues.apache.org/jira/browse/GIRAPH-112 Diffs http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634 Diff: https://reviews.apache.org/r/3287/diff Testing ------- Local unittests and MR unittests. Thanks, Avery
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3287/#review4036
        -----------------------------------------------------------

        Ship it!

        I ran into the same issue yesterday and the solution presented here is correct. For reasons of efficiency, list.elements() returns the internal underlying array for the list, which might be bigger than the number of elements stored in the list. Therefore you should only iterate until list.size() or use the foreachKey() callback.

        • Sebastian

        On 2011-12-21 07:50:20, Avery Ching wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3287/

        -----------------------------------------------------------

        (Updated 2011-12-21 07:50:20)

        Review request for giraph.

        Summary

        -------

        As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity.

        Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases.

        Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java.

        This addresses bug GIRAPH-112.

        https://issues.apache.org/jira/browse/GIRAPH-112

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634

        Diff: https://reviews.apache.org/r/3287/diff

        Testing

        -------

        Local unittests and MR unittests.

        Thanks,

        Avery

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3287/#review4036 ----------------------------------------------------------- Ship it! I ran into the same issue yesterday and the solution presented here is correct. For reasons of efficiency, list.elements() returns the internal underlying array for the list, which might be bigger than the number of elements stored in the list. Therefore you should only iterate until list.size() or use the foreachKey() callback. Sebastian On 2011-12-21 07:50:20, Avery Ching wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3287/ ----------------------------------------------------------- (Updated 2011-12-21 07:50:20) Review request for giraph. Summary ------- As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity. Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases. Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java. This addresses bug GIRAPH-112 . https://issues.apache.org/jira/browse/GIRAPH-112 Diffs ----- http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634 Diff: https://reviews.apache.org/r/3287/diff Testing ------- Local unittests and MR unittests. Thanks, Avery
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/3287/#review4071
        -----------------------------------------------------------

        Ship it!

        Good catch, this looks great, thanks!

        • Jake

        On 2011-12-21 07:50:20, Avery Ching wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/3287/

        -----------------------------------------------------------

        (Updated 2011-12-21 07:50:20)

        Review request for giraph.

        Summary

        -------

        As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity.

        Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases.

        Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java.

        This addresses bug GIRAPH-112.

        https://issues.apache.org/jira/browse/GIRAPH-112

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634

        Diff: https://reviews.apache.org/r/3287/diff

        Testing

        -------

        Local unittests and MR unittests.

        Thanks,

        Avery

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3287/#review4071 ----------------------------------------------------------- Ship it! Good catch, this looks great, thanks! Jake On 2011-12-21 07:50:20, Avery Ching wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3287/ ----------------------------------------------------------- (Updated 2011-12-21 07:50:20) Review request for giraph. Summary ------- As pointed out by YuanYua, the array returned by elements() cannot have its length used since the array contains all the elements currently stored in the mahout collections, even including invalid elements between size and capacity. Whenever possible I converted elements() into forEach(), forEachKey(), forEachPair(). Used size() in other cases. Fixed some formatting violations as well in LongDoubleFloatDoubleVertex.java. This addresses bug GIRAPH-112 . https://issues.apache.org/jira/browse/GIRAPH-112 Diffs ----- http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1221634 Diff: https://reviews.apache.org/r/3287/diff Testing ------- Local unittests and MR unittests. Thanks, Avery
        Hide
        Hudson added a comment -

        Integrated in Giraph-trunk-Commit #58 (See https://builds.apache.org/job/Giraph-trunk-Commit/58/)
        GIRAPH-112: Use elements() properly in LongDoubleFloatDoubleVertex.
        (aching)

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

        • /incubator/giraph/trunk/CHANGELOG
        • /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java
        Show
        Hudson added a comment - Integrated in Giraph-trunk-Commit #58 (See https://builds.apache.org/job/Giraph-trunk-Commit/58/ ) GIRAPH-112 : Use elements() properly in LongDoubleFloatDoubleVertex. (aching) aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1222071 Files : /incubator/giraph/trunk/CHANGELOG /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java
        Hide
        Avery Ching added a comment -

        Thanks for the report and reviews.

        Show
        Avery Ching added a comment - Thanks for the report and reviews.

          People

          • Assignee:
            Avery Ching
            Reporter:
            Yuanyuan Tian
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 5m
              5m
              Remaining:
              Remaining Estimate - 5m
              5m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development