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