incubator-giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (GIRAPH-80) Don't expose the list holding the messages in BasicVertex
Date Thu, 15 Dec 2011 22:41:33 GMT

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

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


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


/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
line 189, 192: broken code conventions, put a new line after you open the block and close
at new line as well. the convention you're using is for fields. Also please explain with one
sentence about the semantic of the parameter and how it is supposed to use it. users might
not read the JIRAs.

For the rest I'm happy with it, nice work.
I don't have availability of real cluster here at the moment, if somebody can launch the testing
on a real cluster I'd be ready to commit this one. 
Otherwise I should be able to run in on monday or so.

- Claudio


On 2011-12-15 10:42:39, Sebastian Schelter wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/3203/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-12-15 10:42:39)
bq.  
bq.  
bq.  Review request for giraph.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  refactoring that gives BasicVertex this 3 new methods:
bq.  
bq.      public abstract Iterable<M> getMessages()
bq.  
bq.  returns an unmodifiable iterable allowing access to the current messages
bq.  
bq.      public abstract void setMessages(Iterable<M> messages);
bq.  
bq.  replacement for getMsgList().clear() followed by getMsgList().addAll(...);
bq.  
bq.      public abstract void releaseResources();
bq.  
bq.  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
bq.  
bq.  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)
bq.  
bq.  
bq.  This addresses bug GIRAPH-80.
bq.      https://issues.apache.org/jira/browse/GIRAPH-80
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    /trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1214675

bq.    /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1214675 
bq.    /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION 
bq.    /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1214675 
bq.    /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION

bq.  
bq.  Diff: https://reviews.apache.org/r/3203/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Sebastian
bq.  
bq.


                
> 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

        

Mime
View raw message