You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Alessandro Presta <al...@fb.com> on 2012/11/02 05:36:06 UTC
Re: Review Request: GIRAPH-390: MasterCompute postApplication callback.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7762/#review13024
-----------------------------------------------------------
I'm not sure how I feel about using MasterCompute for something other than computation. I guess it gets the job done but I wonder if we should have a better place for this sort of code (like a MasterContext or whatever). It's just a matter of separating the roles.
Other than that, the code looks good.
giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java
<https://reviews.apache.org/r/7762/#comment28034>
I think we use the form "vertices" everywhere else in the codebase. Both are considered correct though.
giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java
<https://reviews.apache.org/r/7762/#comment28035>
Mmh, isn't there anything more compact than this?
I guess you could keep the counters in an array/ArrayList, and have constants like SUPERSTEP = 0, VERTICES = 1.
That way you don't even need the inner class, just build an Iterator from the array on the fly.
- Alessandro Presta
On Oct. 31, 2012, 7:43 p.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7762/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 7:43 p.m.)
>
>
> Review request for giraph.
>
>
> Description
> -------
>
> GIRAPH-390: MasterCompute postApplication callback.
>
>
> Diffs
> -----
>
> giraph/src/main/java/org/apache/giraph/counters/GiraphHadoopCounter.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphTimers.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/HadoopCountersBase.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/package-info.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 98a0bd2c6dc9602ea17612d4f68461d5fa43fb00
> giraph/src/main/java/org/apache/giraph/graph/MasterCompute.java 9de23ad4a7781d6342339f26f44fb1c1d9bce2b0
> giraph/src/main/java/org/apache/giraph/graph/MasterThread.java 85842d0ab59330d95b843c440f2d5705b41e0d1b
> giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.java 262db29bbd4797e470f426999b85f2c1a7d1dee3
> giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java ea1cc1ef69aaa2c786b99de51cd30e39ee9f1620
> giraph/src/main/java/org/apache/giraph/metrics/NoOpMetricsRegistry.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/metrics/package-info.java f7529f52527e8e4253f579bd6d01865aa980926b
>
> Diff: https://reviews.apache.org/r/7762/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nitay Joffe
>
>
Re: Review Request: GIRAPH-390: MasterCompute postApplication callback.
Posted by Nitay Joffe <ni...@apache.org>.
> On Nov. 2, 2012, 4:36 a.m., Alessandro Presta wrote:
> > I'm not sure how I feel about using MasterCompute for something other than computation. I guess it gets the job done but I wonder if we should have a better place for this sort of code (like a MasterContext or whatever). It's just a matter of separating the roles.
> > Other than that, the code looks good.
Actually I started it exactly that way but Maja said it's easiser / cleaner to just add the method to MasterCompute rather than create a separate MasterContext object.
> On Nov. 2, 2012, 4:36 a.m., Alessandro Presta wrote:
> > giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java, line 178
> > <https://reviews.apache.org/r/7762/diff/3/?file=183499#file183499line178>
> >
> > Mmh, isn't there anything more compact than this?
> > I guess you could keep the counters in an array/ArrayList, and have constants like SUPERSTEP = 0, VERTICES = 1.
> > That way you don't even need the inner class, just build an Iterator from the array on the fly.
Did same in GiraphTimers too.
- Nitay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7762/#review13024
-----------------------------------------------------------
On Oct. 31, 2012, 7:43 p.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7762/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 7:43 p.m.)
>
>
> Review request for giraph.
>
>
> Description
> -------
>
> GIRAPH-390: MasterCompute postApplication callback.
>
>
> Diffs
> -----
>
> giraph/src/main/java/org/apache/giraph/counters/GiraphHadoopCounter.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphTimers.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/HadoopCountersBase.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/package-info.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 98a0bd2c6dc9602ea17612d4f68461d5fa43fb00
> giraph/src/main/java/org/apache/giraph/graph/MasterCompute.java 9de23ad4a7781d6342339f26f44fb1c1d9bce2b0
> giraph/src/main/java/org/apache/giraph/graph/MasterThread.java 85842d0ab59330d95b843c440f2d5705b41e0d1b
> giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.java 262db29bbd4797e470f426999b85f2c1a7d1dee3
> giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java ea1cc1ef69aaa2c786b99de51cd30e39ee9f1620
> giraph/src/main/java/org/apache/giraph/metrics/NoOpMetricsRegistry.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/metrics/package-info.java f7529f52527e8e4253f579bd6d01865aa980926b
>
> Diff: https://reviews.apache.org/r/7762/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nitay Joffe
>
>
Re: Review Request: GIRAPH-390: MasterCompute postApplication callback.
Posted by Maja Kabiljo <ma...@fb.com>.
> On Nov. 2, 2012, 4:36 a.m., Alessandro Presta wrote:
> > I'm not sure how I feel about using MasterCompute for something other than computation. I guess it gets the job done but I wonder if we should have a better place for this sort of code (like a MasterContext or whatever). It's just a matter of separating the roles.
> > Other than that, the code looks good.
>
> Nitay Joffe wrote:
> Actually I started it exactly that way but Maja said it's easiser / cleaner to just add the method to MasterCompute rather than create a separate MasterContext object.
>
> Alessandro Presta wrote:
> The reason why this is problematic is that MasterCompute is supposed to be used for defining an algorithm (together with Vertex). Say an application uses MC to check some termination condition or working on aggregators.
> If you also want to add stats the way we're doing it, you need to either modify the same MC (mixing algorithm code with instrumentation) or make it subclass a different MC. If you then want to plug that functionality on/off, there is no seamless way.
> I advocate for a clear separation of algorithm code and optional logging/infrastructure code.
> I agree it's slightly easier this way, but I wouldn't say it's cleaner.
We have Vertex computation per vertex and WorkerContext per worker, i.e. many vertices. On master you have just one master.compute, that's why I don't see the clear distinction between MasterCompute and MasterContext, it seems redundant having both. But if you feel it would add value, go ahead. When I suggested using master.compute for everything there were no arguments against it, yours sounds reasonable.
- Maja
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7762/#review13024
-----------------------------------------------------------
On Nov. 2, 2012, 5:53 p.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7762/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2012, 5:53 p.m.)
>
>
> Review request for giraph.
>
>
> Description
> -------
>
> GIRAPH-390: MasterCompute postApplication callback.
>
>
> Diffs
> -----
>
> giraph/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java 69751086ade95c4d645feafa5131587bb9f18206
> giraph/src/main/java/org/apache/giraph/counters/GiraphHadoopCounter.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphTimers.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/HadoopCountersBase.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/package-info.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 98a0bd2c6dc9602ea17612d4f68461d5fa43fb00
> giraph/src/main/java/org/apache/giraph/graph/MasterCompute.java 9de23ad4a7781d6342339f26f44fb1c1d9bce2b0
> giraph/src/main/java/org/apache/giraph/graph/MasterThread.java 85842d0ab59330d95b843c440f2d5705b41e0d1b
> giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.java 262db29bbd4797e470f426999b85f2c1a7d1dee3
> giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java ea1cc1ef69aaa2c786b99de51cd30e39ee9f1620
> giraph/src/main/java/org/apache/giraph/metrics/NoOpMetricsRegistry.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/metrics/package-info.java f7529f52527e8e4253f579bd6d01865aa980926b
>
> Diff: https://reviews.apache.org/r/7762/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nitay Joffe
>
>
Re: Review Request: GIRAPH-390: MasterCompute postApplication callback.
Posted by Alessandro Presta <al...@fb.com>.
> On Nov. 2, 2012, 4:36 a.m., Alessandro Presta wrote:
> > I'm not sure how I feel about using MasterCompute for something other than computation. I guess it gets the job done but I wonder if we should have a better place for this sort of code (like a MasterContext or whatever). It's just a matter of separating the roles.
> > Other than that, the code looks good.
>
> Nitay Joffe wrote:
> Actually I started it exactly that way but Maja said it's easiser / cleaner to just add the method to MasterCompute rather than create a separate MasterContext object.
>
> Alessandro Presta wrote:
> The reason why this is problematic is that MasterCompute is supposed to be used for defining an algorithm (together with Vertex). Say an application uses MC to check some termination condition or working on aggregators.
> If you also want to add stats the way we're doing it, you need to either modify the same MC (mixing algorithm code with instrumentation) or make it subclass a different MC. If you then want to plug that functionality on/off, there is no seamless way.
> I advocate for a clear separation of algorithm code and optional logging/infrastructure code.
> I agree it's slightly easier this way, but I wouldn't say it's cleaner.
>
> Maja Kabiljo wrote:
> We have Vertex computation per vertex and WorkerContext per worker, i.e. many vertices. On master you have just one master.compute, that's why I don't see the clear distinction between MasterCompute and MasterContext, it seems redundant having both. But if you feel it would add value, go ahead. When I suggested using master.compute for everything there were no arguments against it, yours sounds reasonable.
See, my point is also that WorkerContext shouldn't be used for computation.
Workers are an implementation detail of Giraph. The user writing an algorithm shouldn't bother with workers at all. I think we already discussed this when revamping aggregators.
Now when one wants to monitor the infrastructure, like in Nitay's case, it makes sense to work at infrastructure level, i.e. mess with the WorkerContext.
Although, we're now discussing a solution based on the observer pattern, where one can register multiple observers with their callbacks as opposed to sticking everything in a single class.
That is, you have only one Vertex and only one MasterCompute (to define the algorithm), but you can register any number of MasterObserver and WorkerObserver (to run arbitrary code at specific times).
How does this sound?
- Alessandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7762/#review13024
-----------------------------------------------------------
On Nov. 2, 2012, 5:53 p.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7762/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2012, 5:53 p.m.)
>
>
> Review request for giraph.
>
>
> Description
> -------
>
> GIRAPH-390: MasterCompute postApplication callback.
>
>
> Diffs
> -----
>
> giraph/src/main/java/org/apache/giraph/bsp/CentralizedServiceMaster.java 69751086ade95c4d645feafa5131587bb9f18206
> giraph/src/main/java/org/apache/giraph/counters/GiraphHadoopCounter.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphStats.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/GiraphTimers.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/HadoopCountersBase.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/counters/package-info.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 98a0bd2c6dc9602ea17612d4f68461d5fa43fb00
> giraph/src/main/java/org/apache/giraph/graph/MasterCompute.java 9de23ad4a7781d6342339f26f44fb1c1d9bce2b0
> giraph/src/main/java/org/apache/giraph/graph/MasterThread.java 85842d0ab59330d95b843c440f2d5705b41e0d1b
> giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.java 262db29bbd4797e470f426999b85f2c1a7d1dee3
> giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java ea1cc1ef69aaa2c786b99de51cd30e39ee9f1620
> giraph/src/main/java/org/apache/giraph/metrics/NoOpMetricsRegistry.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/metrics/package-info.java f7529f52527e8e4253f579bd6d01865aa980926b
>
> Diff: https://reviews.apache.org/r/7762/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nitay Joffe
>
>