giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alessandro Presta" <alessan...@fb.com>
Subject Re: Review Request: GIRAPH-390: MasterCompute postApplication callback.
Date Fri, 02 Nov 2012 20:23:21 GMT


> 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
> 
>


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