[jira] [Commented] (GIRAPH-80) Don't expose the list holding the messages in BasicVertex

["jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>]

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

jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------


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

Review request for giraph.


Summary
-------

refactoring that gives BasicVertex this 3 new methods:

    public abstract Iterable<M> getMessages()

returns an unmodifiable iterable allowing access to the current messages

    public abstract void setMessages(Iterable<M> messages);

replacement for getMsgList().clear() followed by getMsgList().addAll(...);

    public abstract void releaseResources();

after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used

Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)


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


Diffs
-----

  /trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1214675 
  /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1214675 
  /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION 
  /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1214675 
  /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION 

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


Testing
-------


Thanks,

Sebastian


                
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
>                 Key: GIRAPH-80
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-80
>             Project: Giraph
>          Issue Type: Improvement
>    Affects Versions: 0.70.0
>            Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))

--
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