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 2012/05/09 13:37:47 UTC
Review Request: GIRAPH-20 Move temporary test files from the project
directory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/
-----------------------------------------------------------
Review request for giraph.
Summary
-------
All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
This addresses bug GIRAPH-20.
https://issues.apache.org/jira/browse/GIRAPH-20
Diffs
-----
trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1332106
trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1332106
trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1332106
trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1332106
trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1332106
trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1332106
trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1332106
trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1332106
trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1332106
trunk/src/test/java/org/apache/giraph/BspCase.java 1332106
trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1332106
trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1332106
trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1332106
trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1332106
trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
Diff: https://reviews.apache.org/r/5077/diff
Testing
-------
successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
Thanks,
Sebastian
Re: Review Request: GIRAPH-20 Move temporary test files from the project
directory
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/#review7745
-----------------------------------------------------------
Hey, Sebastian, overall looks good. If no one else gets to it, I'll finish this review tonight.
- Avery
On 2012-05-09 11:37:47, Sebastian Schelter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5077/
> -----------------------------------------------------------
>
> (Updated 2012-05-09 11:37:47)
>
>
> Review request for giraph.
>
>
> Summary
> -------
>
> All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
>
> This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
>
> Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
>
>
> This addresses bug GIRAPH-20.
> https://issues.apache.org/jira/browse/GIRAPH-20
>
>
> Diffs
> -----
>
> trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1332106
> trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1332106
> trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1332106
> trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1332106
> trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1332106
> trunk/src/test/java/org/apache/giraph/BspCase.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1332106
> trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5077/diff
>
>
> Testing
> -------
>
> successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
>
>
> Thanks,
>
> Sebastian
>
>
Re: Review Request: GIRAPH-20 Move temporary test files from the project
directory
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/#review7772
-----------------------------------------------------------
Ship it!
+1, thanks Sebastian! I'm assuming it also passes 'mvn verify', but please double check before you commit.
- Avery
On 2012-05-10 09:32:10, Sebastian Schelter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5077/
> -----------------------------------------------------------
>
> (Updated 2012-05-10 09:32:10)
>
>
> Review request for giraph.
>
>
> Summary
> -------
>
> All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
>
> This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
>
> Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
>
>
> This addresses bug GIRAPH-20.
> https://issues.apache.org/jira/browse/GIRAPH-20
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1336504
> trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1336504
> trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1336504
> trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
> trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1336504
> trunk/src/test/java/org/apache/giraph/BspCase.java 1336504
> trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1336506
> trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1336504
> trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1336504
> trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1336504
> trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1336506
> trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1336504
> trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1336504
> trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1336504
> trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1336504
>
> Diff: https://reviews.apache.org/r/5077/diff
>
>
> Testing
> -------
>
> successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
>
>
> Thanks,
>
> Sebastian
>
>
Re: Review Request: GIRAPH-20 Move temporary test files from the project
directory
Posted by Sebastian Schelter <ss...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/
-----------------------------------------------------------
(Updated 2012-05-10 09:32:10.140325)
Review request for giraph.
Changes
-------
Updated the patch to reflect Avery's comments.
The line FileUtils:87 has to be kept, we initially delete the new file (if it existed) as it will be recreated later.
Summary
-------
All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
This addresses bug GIRAPH-20.
https://issues.apache.org/jira/browse/GIRAPH-20
Diffs (updated)
-----
trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1336504
trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1336504
trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1336504
trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1336504
trunk/src/test/java/org/apache/giraph/BspCase.java 1336504
trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1336506
trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1336504
trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1336504
trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1336504
trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1336506
trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1336504
trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1336504
trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1336504
trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1336504
Diff: https://reviews.apache.org/r/5077/diff
Testing
-------
successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
Thanks,
Sebastian
Re: Review Request: GIRAPH-20 Move temporary test files from the project
directory
Posted by Sebastian Schelter <ss...@apache.org>.
> On 2012-05-10 06:57:01, Avery Ching wrote:
> > Overall, looks great. Can you address the questions/comments and then I'll re-review?
Thanks for the quick review!
I'll address your comments, merge this with the current trunk and posted a new patch.
> On 2012-05-10 06:57:01, Avery Ching wrote:
> > trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java, lines 85-87
> > <https://reviews.apache.org/r/5077/diff/1/?file=108155#file108155line85>
> >
> > Just out of curiosity, why this change?
If one uses writeUTF() and then reads the resulting file with a buffered reader, each line starts with a broken char. Directly writing the bytes out solved this.
> On 2012-05-10 06:57:01, Avery Ching wrote:
> > trunk/src/main/java/org/apache/giraph/utils/FileUtils.java, line 87
> > <https://reviews.apache.org/r/5077/diff/1/?file=108156#file108156line87>
> >
> > Why delete it?
I'll remove this.
> On 2012-05-10 06:57:01, Avery Ching wrote:
> > trunk/src/test/java/org/apache/giraph/TestBspBasic.java, line 242
> > <https://reviews.apache.org/r/5077/diff/1/?file=108160#file108160line242>
> >
> > shouldn't it be 49 not 491?
its a small L not a 1 :)
- Sebastian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/#review7756
-----------------------------------------------------------
On 2012-05-09 11:37:47, Sebastian Schelter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5077/
> -----------------------------------------------------------
>
> (Updated 2012-05-09 11:37:47)
>
>
> Review request for giraph.
>
>
> Summary
> -------
>
> All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
>
> This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
>
> Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
>
>
> This addresses bug GIRAPH-20.
> https://issues.apache.org/jira/browse/GIRAPH-20
>
>
> Diffs
> -----
>
> trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1332106
> trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1332106
> trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1332106
> trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1332106
> trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1332106
> trunk/src/test/java/org/apache/giraph/BspCase.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1332106
> trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5077/diff
>
>
> Testing
> -------
>
> successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
>
>
> Thanks,
>
> Sebastian
>
>
Re: Review Request: GIRAPH-20 Move temporary test files from the project
directory
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5077/#review7756
-----------------------------------------------------------
Overall, looks great. Can you address the questions/comments and then I'll re-review?
trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java
<https://reviews.apache.org/r/5077/#comment17058>
Just out of curiosity, why this change?
trunk/src/main/java/org/apache/giraph/utils/FileUtils.java
<https://reviews.apache.org/r/5077/#comment17059>
Why delete it?
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17060>
Empty params and return.
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17061>
Empty params and return.
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17062>
Empty params and return.
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17063>
Empty params and return.
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17064>
@return
trunk/src/test/java/org/apache/giraph/BspCase.java
<https://reviews.apache.org/r/5077/#comment17065>
@return
trunk/src/test/java/org/apache/giraph/TestBspBasic.java
<https://reviews.apache.org/r/5077/#comment17066>
shouldn't it be 49 not 491?
- Avery
On 2012-05-09 11:37:47, Sebastian Schelter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5077/
> -----------------------------------------------------------
>
> (Updated 2012-05-09 11:37:47)
>
>
> Review request for giraph.
>
>
> Summary
> -------
>
> All temporary files that the tests generate are now written to /tmp/_giraphTests including zooKeeper files, checkpoints etc.
>
> This behavior will be automatically configured whenever InternalVertexRunner.run() or BspCase.prepareJob() is used.
>
> Usually I can't stop myself once I have my refactoring hat on, therefore I also tidied up a lot of minor stuff, removed code duplications etc.
>
>
> This addresses bug GIRAPH-20.
> https://issues.apache.org/jira/browse/GIRAPH-20
>
>
> Diffs
> -----
>
> trunk/src/test/java/org/apache/giraph/TestZooKeeperExt.java 1332106
> trunk/src/test/java/org/apache/giraph/graph/TestEdgeListVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestJsonBase64Format.java 1332106
> trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java 1332106
> trunk/src/test/java/org/apache/giraph/TestNotEnoughMapTasks.java 1332106
> trunk/src/test/java/org/apache/giraph/TestGraphPartitioner.java 1332106
> trunk/src/test/java/org/apache/giraph/TestAutoCheckpoint.java 1332106
> trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1332106
> trunk/src/test/java/org/apache/giraph/BspCase.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java 1332106
> trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1332106
> trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java 1332106
> trunk/src/main/java/org/apache/giraph/utils/FileUtils.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5077/diff
>
>
> Testing
> -------
>
> successfully passed local and pseudo-distributed tests with Hadoop 0.20.203
>
>
> Thanks,
>
> Sebastian
>
>