You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Sebastian Schelter <ss...@apache.org> on 2011/12/15 11:42:39 UTC
Review Request: Refactor vertices to not expose the internal datastructure
for holding messages
-----------------------------------------------------------
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
Re: Review Request: Refactor vertices to not expose the internal
datastructure for holding messages
Posted by Claudio Martella <cl...@gmail.com>.
-----------------------------------------------------------
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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3203/
> -----------------------------------------------------------
>
> (Updated 2011-12-15 10:42:39)
>
>
> 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
>
>
Re: Review Request: GIRAPH-80 Refactor vertices to not expose the internal
datastructure for holding messages
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/#review3967
-----------------------------------------------------------
Ship it!
+1. Thanks for the changes. I will commit and then open up a separate JIRA to make setMessages() package-private.
- Avery
On 2011-12-17 09:36:06, Sebastian Schelter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3203/
> -----------------------------------------------------------
>
> (Updated 2011-12-17 09:36:06)
>
>
> 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 1215442
> /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
> /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
> /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
> /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
> /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3203/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sebastian
>
>
Re: Review Request: GIRAPH-80 Refactor vertices to not expose the internal
datastructure for holding messages
Posted by Sebastian Schelter <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/
-----------------------------------------------------------
(Updated 2011-12-17 09:36:06.229668)
Review request for giraph.
Summary (updated)
-------
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 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/3203/diff
Testing
-------
Thanks,
Sebastian
Re: Review Request: Refactor vertices to not expose the internal
datastructure for holding messages
Posted by Sebastian Schelter <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/
-----------------------------------------------------------
(Updated 2011-12-17 09:35:51.778265)
Review request for giraph.
Changes
-------
Updated the patch to include proposed changes. Unfortunately setMessages(...) cannot be package-private as it is invoked by BasicRPCCommunications.
Ran tests in local and pseudo-distributed mode.
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 (updated)
-----
/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/3203/diff
Testing
-------
Thanks,
Sebastian
Re: Review Request: Refactor vertices to not expose the internal
datastructure for holding messages
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3203/
> -----------------------------------------------------------
>
> (Updated 2011-12-15 10:42:39)
>
>
> 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
>
>