You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Avery Ching <av...@gmail.com> on 2013/06/27 01:48:02 UTC
Review Request 12125: GIRAPH-698
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/
-----------------------------------------------------------
Review request for giraph.
Bugs: GIRAPH-698
https://issues.apache.org/jira/browse/GIRAPH-698
Repository: giraph-git
Description
-------
* Prints out the computation name with the superstep for easier debugging
* Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
Diffs
-----
giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
Diff: https://reviews.apache.org/r/12125/diff/
Testing
-------
* Ran PageRankBenchmark on a real cluster (output attached to JIRA)
* mvn clean verify
Thanks,
Avery Ching
Re: Review Request 12125: GIRAPH-698
Posted by Avery Ching <av...@gmail.com>.
> On June 27, 2013, midnight, Alessandro Presta wrote:
> > giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java, line 1647
> > <https://reviews.apache.org/r/12125/diff/2/?file=311354#file311354line1647>
> >
> > You want to avoid checking for previous outgoing message type = incoming message type, but what about other checks (e.g., combiner message type)? Aren't those valid for superstep 0 too?
Will fix this.
- Avery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22442
-----------------------------------------------------------
On June 26, 2013, 11:48 p.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 26, 2013, 11:48 p.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22442
-----------------------------------------------------------
giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/12125/#comment45996>
You want to avoid checking for previous outgoing message type = incoming message type, but what about other checks (e.g., combiner message type)? Aren't those valid for superstep 0 too?
- Alessandro Presta
On June 26, 2013, 11:48 p.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 26, 2013, 11:48 p.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Alessandro Presta <al...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22452
-----------------------------------------------------------
Looks good to me now. Nitay?
giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java
<https://reviews.apache.org/r/12125/#comment46002>
Question mark is not needed :)
- Alessandro Presta
On June 27, 2013, 12:36 a.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 27, 2013, 12:36 a.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
> giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java 13bb492062a98687a375d36ce3c7c3f2ec8a986e
> giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java 8ae09bc13471afc5a7d6e6b548d6110e1edfb4cc
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Nitay Joffe <ni...@apache.org>.
> On June 27, 2013, 1:01 a.m., Nitay Joffe wrote:
> > giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java, line 137
> > <https://reviews.apache.org/r/12125/diff/3/?file=312477#file312477line137>
> >
> > What is this check? Looks odd should be explained
>
> Avery Ching wrote:
> I need the null checks for the master since we don't have the classes set at that time -
> which is reasonable, this avoids an NPE. I think the behavior is reasonable given that the classes aren't set prior to BspServiceMaster#coordinateSuperstep.
Okay, please add a comment about this. Otherwise +1.
- Nitay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22456
-----------------------------------------------------------
On June 27, 2013, 12:36 a.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 27, 2013, 12:36 a.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
> giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java 13bb492062a98687a375d36ce3c7c3f2ec8a986e
> giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java 8ae09bc13471afc5a7d6e6b548d6110e1edfb4cc
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Avery Ching <av...@gmail.com>.
> On June 27, 2013, 1:01 a.m., Nitay Joffe wrote:
> > giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java, line 137
> > <https://reviews.apache.org/r/12125/diff/3/?file=312477#file312477line137>
> >
> > What is this check? Looks odd should be explained
I need the null checks for the master since we don't have the classes set at that time -
which is reasonable, this avoids an NPE. I think the behavior is reasonable given that the classes aren't set prior to BspServiceMaster#coordinateSuperstep.
> On June 27, 2013, 1:01 a.m., Nitay Joffe wrote:
> > giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java, line 159
> > <https://reviews.apache.org/r/12125/diff/3/?file=312477#file312477line159>
> >
> > As above
Same as above.
- Avery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22456
-----------------------------------------------------------
On June 27, 2013, 12:36 a.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 27, 2013, 12:36 a.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
> giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java 13bb492062a98687a375d36ce3c7c3f2ec8a986e
> giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java 8ae09bc13471afc5a7d6e6b548d6110e1edfb4cc
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Nitay Joffe <ni...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22456
-----------------------------------------------------------
Overall LG, just lots of null checks in this diff are a bit worrisome, any way around that?
giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java
<https://reviews.apache.org/r/12125/#comment46008>
What is this check? Looks odd should be explained
giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java
<https://reviews.apache.org/r/12125/#comment46009>
As above
- Nitay Joffe
On June 27, 2013, 12:36 a.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 27, 2013, 12:36 a.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
> giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java 13bb492062a98687a375d36ce3c7c3f2ec8a986e
> giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java 8ae09bc13471afc5a7d6e6b548d6110e1edfb4cc
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/
-----------------------------------------------------------
(Updated June 27, 2013, 12:36 a.m.)
Review request for giraph.
Changes
-------
More precise checking on types for superstep 0.
Bugs: GIRAPH-698
https://issues.apache.org/jira/browse/GIRAPH-698
Repository: giraph-git
Description
-------
* Prints out the computation name with the superstep for easier debugging
* Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
Diffs (updated)
-----
giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java 13bb492062a98687a375d36ce3c7c3f2ec8a986e
giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java 8ae09bc13471afc5a7d6e6b548d6110e1edfb4cc
Diff: https://reviews.apache.org/r/12125/diff/
Testing
-------
* Ran PageRankBenchmark on a real cluster (output attached to JIRA)
* mvn clean verify
Thanks,
Avery Ching
Re: Review Request 12125: GIRAPH-698
Posted by Avery Ching <av...@gmail.com>.
> On June 27, 2013, 12:13 a.m., Nitay Joffe wrote:
> > Why not use GiraphComputation#getComputationName which works for things like Jython as well?
Seemed cleaner to have a method to get the MasterCompute from the BspServiceMaster rather than the name. We do not expose the GiraphConfiguration in BspMasterCompute, that would be another way I guess. I don't have a strong preference on this.
- Avery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22447
-----------------------------------------------------------
On June 26, 2013, 11:48 p.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 26, 2013, 11:48 p.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>
Re: Review Request 12125: GIRAPH-698
Posted by Nitay Joffe <ni...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12125/#review22447
-----------------------------------------------------------
Why not use GiraphComputation#getComputationName which works for things like Jython as well?
- Nitay Joffe
On June 26, 2013, 11:48 p.m., Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12125/
> -----------------------------------------------------------
>
> (Updated June 26, 2013, 11:48 p.m.)
>
>
> Review request for giraph.
>
>
> Bugs: GIRAPH-698
> https://issues.apache.org/jira/browse/GIRAPH-698
>
>
> Repository: giraph-git
>
>
> Description
> -------
>
> * Prints out the computation name with the superstep for easier debugging
> * Fixes a minor bug with MasterCompute for differing message types for superstep 0 and if halted
>
>
> Diffs
> -----
>
> giraph-core/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java f41fc3d54c5a6d4b2e7278afcd030748e3949e98
> giraph-core/src/main/java/org/apache/giraph/counters/GiraphTimers.java 0d50c29baf1087056c49c08d86313594e0417529
> giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java 355888785d32d743837056905a16dc2f75a6f035
> giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java 310cb26106baa7eec85f16928a6fee66050d08c5
> giraph-core/src/main/java/org/apache/giraph/master/MasterThread.java e8eeeed61455c471986c72c21023409aa4b8fb6e
>
> Diff: https://reviews.apache.org/r/12125/diff/
>
>
> Testing
> -------
>
> * Ran PageRankBenchmark on a real cluster (output attached to JIRA)
> * mvn clean verify
>
>
> Thanks,
>
> Avery Ching
>
>