[
https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170645#comment-13170645
]
jirapos...@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/#review3936
-----------------------------------------------------------
I think that overall this looks pretty nice. I do have a couple of
suggestions. Also in the CODE_CONVENTIONS, comments should start with a
capital letter i.e. (// This convention is silly).
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
<https://reviews.apache.org/r/3203/#comment8881>
Should be package-private to avoid the user from mucking around with the
message data structure.
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
<https://reviews.apache.org/r/3203/#comment8884>
Should be package-private to only be called by the infrastructure.
Can we capitalize the comments? I.e. /** Release...
Also the comment is not quite right. releaseResources() will be called
after the computation of the vertex, not only after a halted vertex.
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java
<https://reviews.apache.org/r/3203/#comment8880>
Thanks for fixing this (my bad)! Argh.
- Avery
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
