You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Veselin Stoyanov <ve...@fb.com> on 2013/03/20 22:56:27 UTC
Review Request: In memory testing framework added
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
Review request for giraph.
Description
-------
A framework for running small test graphs in memory.
Diffs
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
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
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18427
-----------------------------------------------------------
Looking good. See inline comments.
Further suggestions for eliminating the intermediate map storage:
- the internal storage for TestGraph will be a Map<I, Vertex>
- addVertex(I, V) will add a Vertex with the given id and value and no edges, using the GiraphClasses instance
- addVertex(I, V, Iterable<Edge<I, E>>) will add a Vertex with the given edges
- addEdge(I, I, E) will add an edge from the first vertex to the second vertex, with the given edge value; if the source vertex doesn't exist, it will be created
- TestGraph will implement Iterable<Vertex> instead of Iterable<I>
- an additional getVertex(I) method will be used for random access
- the InMemoryVertexInputFormat will simply pass references to the already-created vertices
- no output format is needed; run() will return the same TestGraph instance
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38637>
I would probably rename this class to InMemoryVertexInputFormat and change the comment to "An input format that reads the input graph in memory. Used for unit tests." or something like that.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38638>
Can you move this method down, close to setConf()?
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38635>
Missing a newline.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38639>
Style: we prefer more explicit/verbose naming here.
i -> id
v -> value
es -> edges
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38636>
Extra newline.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38640>
is -> inputSplit
c - > context
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38641>
I would rename this to something like idIterator.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38642>
c -> context
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38643>
InMemoryVertexOutputFormat?
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38645>
Extra newline.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38644>
Can't you use the ImmutableOutputCommitter here?
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38646>
Maps.newHashMap() is more compact.
This applies to pretty much all collections here (Lists.newArrayList() and so on).
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38647>
neighbours -> edges
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38648>
"Iterator over the edges for a given vertex id"
- Alessandro Presta
On March 26, 2013, 11:30 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 26, 2013, 11:30 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/InMemoryVertexInput.java PRE-CREATION
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18482
-----------------------------------------------------------
Please address these last few suggestions which you may have missed from the previous reviews.
And yes, my dream is close to coming true!
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java
<https://reviews.apache.org/r/10052/#comment38771>
Why not return the same TestGraph instance instead?
If TestGraph implements Iterable<Vertex> and has getVertex(i), that's basically all we need, and that way input and output talk the same class.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38770>
You're not using "iterability" of TestGraph anywhere.
If you make this an Iterable<Vertex> though, it can be pretty useful.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38765>
Not sure why clear() needs to return this. What is the use case?
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38768>
You're not using this id iterator anywhere, so why not make this the vertex iterator instead?
giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38766>
Again, why no generic types here?
- Alessandro Presta
On March 28, 2013, 10: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, 10: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/InMemoryVertexInputFormat.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18520
-----------------------------------------------------------
Ship it!
I'm still not sure why you need to include an id iterator, but I'm fine with this.
Can you post an updated patch on the JIRA (create with "post-review -n > GIRAPH-577.patch").
Thanks!
- Alessandro Presta
On March 29, 2013, 5:56 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 29, 2013, 5:56 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/InMemoryVertexInputFormat.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 29, 2013, 5:56 p.m.)
Review request for giraph.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInputFormat.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
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
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18492
-----------------------------------------------------------
One last thing you forgot to address, then we can ship.
giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38784>
You forgot to make TestGraph implement Iterable<Vertex> instead of Iterable<I>, which will make this pattern a bit cleaner:
for (Vertex<I, V, E, M> vertex : results) {
components.put(vertex.getValue().get(), vertex.getId().get());
}
I would only use getVertex() when we need to check a condition against a specific vertex.
- Alessandro Presta
On March 29, 2013, 4:34 a.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 29, 2013, 4:34 a.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/InMemoryVertexInputFormat.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 29, 2013, 4:34 a.m.)
Review request for giraph.
Changes
-------
This should be the new version now.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInputFormat.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 28, 2013, 11:08 p.m.)
Review request for giraph.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInputFormat.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 28, 2013, 10:19 p.m.)
Review request for giraph.
Changes
-------
Hopefully this should do it!
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInputFormat.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 4b03127f3490d35e6e3ddf7b69a9d0bc4673420d
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
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.
Changes
-------
Alessandro gets his dream. TestGraph stores vertices now (instead of Maps of values and edges).
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 26, 2013, 11:30 p.m.)
Review request for giraph.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 26, 2013, 11:28 p.m.)
Review request for giraph.
Changes
-------
Fixed configuration to be passed to InMemoryVertexInput.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
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
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 26, 2013, 9:25 p.m.)
Review request for giraph.
Changes
-------
Implemented changes suggested by Alessandro.
Description
-------
A framework for running small test graphs in memory.
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
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
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java, line 129
> > <https://reviews.apache.org/r/10052/diff/1/?file=272631#file272631line129>
> >
> > I would call this "getEdges", maybe "createEdges". Also, looks like it could be converted to a static method.
>
> Veselin Stoyanov wrote:
> I changed the name, but if I make it static, I can't use the generic types.
Can't you make it a generic method?
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java, line 170
> > <https://reviews.apache.org/r/10052/diff/1/?file=272631#file272631line170>
> >
> > Assigning the format's conf from the reader doesn't feel right.
> > You can instead make the format implement ImmutableClassesGiraphConfigurable, so that Giraph will provide it with the conf upon instantiation.
>
> Veselin Stoyanov wrote:
> I was going by the way that this is done in io.formats.TextVertexInputFormat (line 101). I can change it if you think it's better.
Yes, I think it's better. A member class setting a field of the enclosing class seems error-prone.
- Alessandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18177
-----------------------------------------------------------
On March 26, 2013, 9:25 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 26, 2013, 9:25 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/InMemoryEdgeInput.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java, line 66
> > <https://reviews.apache.org/r/10052/diff/1/?file=272634#file272634line66>
> >
> > It would be useful to also have an addEdge(I sourceVertexId, targetVertexId) method.
> > If the vertex exists, it should add the edge. If the vertex doesn't exist, it should create it with a default value and then add the edge.
I meant addEdge(I sourceVertexId, Edge<I, E> edge).
- Alessandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18177
-----------------------------------------------------------
On March 20, 2013, 10:05 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 10:05 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/InMemoryEdgeInput.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java, line 129
> > <https://reviews.apache.org/r/10052/diff/1/?file=272631#file272631line129>
> >
> > I would call this "getEdges", maybe "createEdges". Also, looks like it could be converted to a static method.
I changed the name, but if I make it static, I can't use the generic types.
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java, line 42
> > <https://reviews.apache.org/r/10052/diff/1/?file=272634#file272634line42>
> >
> > I think we should be able to skip this intermediate representation, and directly store the vertices in a Map<I, Vertex>.
> > Then, InMemoryVertexInput can simply return references to the already-created vertices in TestGraph.
> > No output format will be needed then, as the vertices will be modified in place.
> > That way InternalVertexRunner#run() can simply return the same TestGraph reference, and a user can do any checks directly against that.
That sounds right, but given time constraints and the fact that the current implementation works, I'll leave it as it is for now.
> On March 20, 2013, 10:56 p.m., Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java, line 170
> > <https://reviews.apache.org/r/10052/diff/1/?file=272631#file272631line170>
> >
> > Assigning the format's conf from the reader doesn't feel right.
> > You can instead make the format implement ImmutableClassesGiraphConfigurable, so that Giraph will provide it with the conf upon instantiation.
I was going by the way that this is done in io.formats.TextVertexInputFormat (line 101). I can change it if you think it's better.
- Veselin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18177
-----------------------------------------------------------
On March 20, 2013, 10:05 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 10:05 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/InMemoryEdgeInput.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/#review18177
-----------------------------------------------------------
Thanks for taking the time to work on this!
Please see the inline comments.
My main concern is that I don't see how we get edge values. It seems to me that there should be no need for InMemoryEdgeInput, and instead we should store edge values in TestGraph (probably adding an addEdge() method too).
Also, I think this functionality could be achieved more simply by storing the Vertex objects in TestGraph (probably in a Map<I, Vertex>), and having InMemoryVertexInput simply return references to those vertices. There will be no need for InMemoryVertexOutput, as the vertices will be modified in-place and InternalVertexRunner#run() will just return the same TestGraph.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java
<https://reviews.apache.org/r/10052/#comment38346>
Usually we put this all in one line.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java
<https://reviews.apache.org/r/10052/#comment38349>
Why is this class needed anyway? Isn't a VertexInputFormat sufficient once we have built our TestGraph? You just need to handle edge values in there too.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java
<https://reviews.apache.org/r/10052/#comment38347>
Why do you need your own DummyInputSplit? Can't you use BspInputSplit like GeneratedVertexInputFormat does?
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java
<https://reviews.apache.org/r/10052/#comment38359>
Why is this in terms of WritableComparable and Writable instead of I and E?
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java
<https://reviews.apache.org/r/10052/#comment38348>
What's going on with this iterator? You get it from an empty list. Later you have a commented assignment.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38350>
See above.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38351>
See above.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38352>
I would call this "getEdges", maybe "createEdges". Also, looks like it could be converted to a static method.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38353>
Why are we assuming there are no edge values?
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38354>
Leftover.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java
<https://reviews.apache.org/r/10052/#comment38355>
Assigning the format's conf from the reader doesn't feel right.
You can instead make the format implement ImmutableClassesGiraphConfigurable, so that Giraph will provide it with the conf upon instantiation.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38356>
See above.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38360>
I think this would be easier to use if it returned a Map<I, Vertex> instead. That way we can easily look up just the vertices we're interested in, as well as iterate if we want to check a condition against all vertices.
Even better would be to have TestGraph itself implement a sort of map-like interface providing lookup and iteration over values, and skip the output format entirely.
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java
<https://reviews.apache.org/r/10052/#comment38357>
There's a class called ImmutableOutputCommitter (not sure why it's called like that) that's the same as this anonymous class.
giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java
<https://reviews.apache.org/r/10052/#comment38358>
This comment is not completely relevant, since there is no input file or output folder, right?
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38366>
This is really a container of vertex values, not vertices.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38364>
I think we should be able to skip this intermediate representation, and directly store the vertices in a Map<I, Vertex>.
Then, InMemoryVertexInput can simply return references to the already-created vertices in TestGraph.
No output format will be needed then, as the vertices will be modified in place.
That way InternalVertexRunner#run() can simply return the same TestGraph reference, and a user can do any checks directly against that.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38367>
This is really a container of neighbor ids, not edges.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38370>
Can you switch to more descriptive parameter names, as in the rest of the codebase?
In this case, since it's clear that we're talking about a vertex, I'd be happy with "id" and "value" instead of "i" and "v".
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38369>
It would be useful to also have an addEdge(I sourceVertexId, targetVertexId) method.
If the vertex exists, it should add the edge. If the vertex doesn't exist, it should create it with a default value and then add the edge.
giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java
<https://reviews.apache.org/r/10052/#comment38365>
This is a bit misleading because it doesn't return an iterator over the edges, but over the neighbor ids.
Also, unless you need it to be an ArrayList in the caller, you can return a more generic Iterable<I>.
giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38361>
Trailing space.
giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38368>
Can't you rewrite this more compactly as a chain of calls?
graph.addVertex(...)
.addVertex(...)
.addVertex(...)
...
giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsVertexTestInMemory.java
<https://reviews.apache.org/r/10052/#comment38362>
Trailing spaces.
- Alessandro Presta
On March 20, 2013, 10:05 p.m., Veselin Stoyanov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10052/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 10:05 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/InMemoryEdgeInput.java PRE-CREATION
> giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
> 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
>
>
Re: Review Request: In memory testing framework added
Posted by Veselin Stoyanov <ve...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10052/
-----------------------------------------------------------
(Updated March 20, 2013, 10:05 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/InMemoryEdgeInput.java PRE-CREATION
giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java PRE-CREATION
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