[ 
https://issues.apache.org/jira/browse/GIRAPH-124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13190625#comment-13190625
 ] 

Avery Ching commented on GIRAPH-124:
------------------------------------

Thanks for the improvements, I have a few more comments.  These comments are a 
little tricky on JIRA, do you want to try reviewboard next time?

{noformat} 
+   /**
+    * Combines message values for a particular vertex index.
+    *
+    * @param vertexIndex Index of the vertex getting these messages
+    * @param messages Iterable of the messages to be combined
+    * @return Message that is combined from {@link messages} or null if no
+    *         message it to be sent
+    * @throws IOException
+    */
+    public abstract Iterable<M> combine(I vertexIndex,
+            Iterable<M> messages) throws IOException;
{noformat} 

This javadoc is not quite right.  @return should be corrected, for instance, 
null is not legal now right?

{noformat}
+                        Iterable<M> messages = combiner.combine(entry.getKey(),
                                                          entry.getValue());
{noformat}

Can you align 'entry.getValue()'?

{noformat}
+                    if (Iterables.size(outMessageList) < 
+                            Iterables.size(messages)) {
{noformat}

Before you do this check, we still need to do a check that messages is not 
null.  If it is null, the combiner has violated the combine() contract.  We 
need to throw an exception at this point.

Also, have you run the unittests (local and MR)?

Thanks!

                
> Combiner should return Iterable<M> instead of M or null.
> --------------------------------------------------------
>
>                 Key: GIRAPH-124
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-124
>             Project: Giraph
>          Issue Type: Improvement
>          Components: graph
>    Affects Versions: 0.1.0
>            Reporter: Claudio Martella
>         Attachments: GIRAPH-124.diff, GIRAPH-124.diff
>
>
> Currently VertexCombiner is expected to return a single message combining the 
> input messages, or null in case no message should be sent. The new expected 
> interface should return an Iterable<M>, possibly empty. The number of 
> elements in the returned Iterable is supposed to be smaller than the number 
> of input messages, by the initial definition of a Combiner (defined as a 
> function to reduce I/O by combining multiple messages into 1).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to