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