giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alessandro Presta" <alessan...@fb.com>
Subject Re: Review Request: In memory testing framework added
Date Thu, 28 Mar 2013 21:00:33 GMT

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


I think we're almost done!


giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38732>

    I think you can still do without an output format: why do you need to store the vertices
in a new hash map?
    Shouldn't it work just fine to inspect the TestGraph instance?
    You can make InternalVertexRunner.run() return the same TestGraph although it's not strictly
needed, since the user can just check the instance passed to run().



giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java
<https://reviews.apache.org/r/10052/#comment38717>

    Make sure you are setting the same classes as in the other version of run().
    I think this one misses the recently added inputVertexEdges and vertexValueFactory.
    Sorry, the current situation sucks, run() should really take a GiraphConfiguration instead.



giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38721>

    I think the idea here should be to default-create the vertex (calling conf.createVertex()
and then initialize(id, conf.createVertexValue()).
    This is consistent with edge-based input formats.



giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38722>

    Why not provide an iterator over the vertices instead? The id will be in vertex.getId()
anyway, and this way it takes less code (and less lookups) to iterate over all vertices.



giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38730>

    Why is this public?



giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38718>

    Please remove this comment, it was just an explanation I put in the snippet :)
    Also, I think you need to set the VertexValueFactory too.



giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38719>

    Extra whitespace.



giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38731>

    Why no type arguments?


- Alessandro Presta


On March 28, 2013, 6:19 p.m., Veselin Stoyanov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
> 
> (Updated March 28, 2013, 6:19 p.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> A framework for running small test graphs in memory.
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 7e0b955998a5f1890912819fa2ca74fbbf46fedd

>   giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java PRE-CREATION 
>   giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/10052/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Veselin Stoyanov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message