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

jirapos...@reviews.apache.org commented on GIRAPH-116:
------------------------------------------------------



bq.  On 2012-01-02 11:04:25, Sebastian Schelter wrote:
bq.  > Had a quick look over your changes and everything looked good. I think 
it's right to assume that most implementations will use static graphs and to 
offer EdgeListVertex as the default extension point for this. The only thing I 
don't like is the name change from MutableVertex to BasicVertex, I liked the 
former better because it sounds much more expressive to me.

Sebastian, I didn't change the name from MutableVertex to BasicVertex, sorry 
for the mixup.  I think MutableVertex is useful as well.  The only thing I did 
chance for for the addVertex() and addVertexReq() methods to take a BasicVertex 
rather than a MutableVertex to be a tad safer.  We still have the BasicVertex 
-> MutableVertex -> User vertex class hierarchy.


bq.  On 2012-01-02 11:04:25, Sebastian Schelter wrote:
bq.  > 
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java,
 line 61
bq.  > <https://reviews.apache.org/r/3349/diff/2/?file=66112#file66112line61>
bq.  >
bq.  >     good thing to have the Iterable<> abstraction here

=)


- Avery


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


On 2012-01-02 02:35:50, Avery Ching wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/3349/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-01-02 02:35:50)
bq.  
bq.  
bq.  Review request for giraph.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  * Changed Vertex.java to HashMapVertex.java.  This makes it less likely 
folks will use it as a default.  I have included comments that suggest 
EdgeListVertex for static graphs (most cases).
bq.  
bq.  * Found and fixed bugs in EdgeListVertex with the way that binarySearch 
was being used.  Added unittests to check for adding/getting/removing edges.
bq.  
bq.  * Changed classes that extend Vertex to extending EdgeListVertex instead.
bq.  
bq.  * Changed MutableVertex to BasicVertex for addVertex, addVertexReq to be a 
little safer
bq.  
bq.  * Tried to make sure that when a class that extends MutableVertex is 
instantiated that it also will call readFields() or initialize().  This fixed 
several bugs.
bq.  
bq.  * Changed the interface of BasicVertex#initialize from  public abstract 
void initialize(I vertexId, V vertexValue, Map<I, E> edges, List<M> messages) 
to 
bq.  initialize(I vertexId, V vertexValue, Map<I, E> edges, Iterable<M> 
messages) to better fit the recent changes to BasicVertex getting/setting 
messages with an Iterable.
bq.  
bq.  * Found and removed duplicated code from several MutableVertex extended 
classes for addVertexRequest, removeVertexRequest, addEdgeRequest and 
removeEdgeRequest.
bq.  
bq.  * Changed Vertex cast to BasicVertex cast in Partition and MockUtils.
bq.  
bq.  * There are some tabs --> spaces conversions done automatically from my 
Ecipse settings for the files I touched.
bq.  
bq.  
bq.  This addresses bug GIRAPH-116.
bq.      https://issues.apache.org/jira/browse/GIRAPH-116
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/benchmark/PageRankBenchmark.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/benchmark/PseudoRandomVertexInputFormat.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/CommunicationsInterface.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/WorkerCommunications.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCombinerVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleFailVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMsgVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMutateGraphVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleShortestPathsVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleSuperstepVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleVertexWithWorkerContext.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/VerifyMessage.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/HashMapVertex.java
 PRE-CREATION 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/IntIntNullIntVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/MutableVertex.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/partition/Partition.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestVertexTypes.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/examples/SimpleShortestPathVertexTest.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/lib/TestLongDoubleDoubleAdjacencyListVertexInputFormat.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/lib/TestTextDoubleDoubleAdjacencyListVertexInputFormat.java
 1226330 
bq.    
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/utils/MockUtils.java
 1226330 
bq.  
bq.  Diff: https://reviews.apache.org/r/3349/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Passed unittests and pseudo-distributed MR tests.  Passed newly added 
unittests as well for EdgeListVertex.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Avery
bq.  
bq.


                
> Make EdgeListVertex the default vertex implementation, fix bugs related to 
> EdgeListVertex.
> ------------------------------------------------------------------------------------------
>
>                 Key: GIRAPH-116
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-116
>             Project: Giraph
>          Issue Type: Improvement
>            Reporter: Avery Ching
>            Assignee: Avery Ching
>         Attachments: GIRAPH-116.patch
>
>
> I think this would best for new users as it is much more memory efficient 
> than Vertex with respect to edges (list vs hash map).  We seem to be mostly 
> iterating over the edges (as several others had pointed out in earlier JIRAs 
> and emails), so this would provide early users with a more memory efficient 
> implementation without performance loss.  If anyone disagrees, please voice 
> your opinions.

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

        

Reply via email to