> On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > Looks great! Aggregators are an important feature that we need to get > > right, and this integrates well with the new MasterCompute. > > > > Just a few questions/ideas about design, but this already looks solid.
Thanks for looking into it, Alessandro! > On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BasicAggregator.java, > > line 30 > > <https://reviews.apache.org/r/6134/diff/1/?file=128731#file128731line30> > > > > I'm most likely missing something, but what prevents us from baking > > this functionality into Aggregator (thus making it an abstract class, much > > like Vertex) as opposed to having an interface and an abstract > > implementation? I did it this way because it gives user an option to do it in a different way. I can't think right now of a use case which would need it, though. > On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorWrapper.java, > > line 67 > > <https://reviews.apache.org/r/6134/diff/1/?file=128768#file128768line67> > > > > I would rename this to current. Previous is the (completely aggregated) > > one we read, current is the (partially aggregated) one we update. What do > > you think? That's how I named it at first, but I thought this is clearer. When you just see getCurrentAggregator without looking into other methods, one might misinterpret it. > On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceMaster.java, > > line 1564 > > <https://reviews.apache.org/r/6134/diff/1/?file=128771#file128771line1564> > > > > Style: can you format this nicely for future readers? > > For example, put "Iterables.transform" altogether (you can break after > > "=" if it doesn't fit) and move the "new Function" argument to a separate > > line instead of breaking it. Will do, thanks! > On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorUsage.java, > > line 74 > > <https://reviews.apache.org/r/6134/diff/1/?file=128767#file128767line74> > > > > Again, you've probably already thought about this, but just curious: > > what are the pros/cons of giving the user an instance of Aggregator instead > > of accessing them via their names like we're doing here? We can't give him Aggregator instance, since we don't want him to have all the functionalities which it has. The only thing we could do is make another interface (don't know what we would call it :-)) with just aggregate(value) and getAggregatedValue() methods, and make AggregatorWrapper implement it. But still he wouldn't be able to cast this object to his Aggregator class. > On July 25, 2012, 12:26 p.m., Alessandro Presta wrote: > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/Aggregator.java, > > line 36 > > <https://reviews.apache.org/r/6134/diff/1/?file=128766#file128766line36> > > > > Just an idea, what do you think of this alternative interface: > > > > A aggregate(A current, A value) > > > > A getInitialValue() > > > > This would make them a bit similar to Combiners, in that the user > > defines an operation and a neutral element, not the internals to change the > > state. > > In other words, the user doesn't call get/setValue, that's taken care > > by the infrastructure. > > > > Would this complicate the design? createAggregatedValue() is used for getting an instance of aggregate object so we could read it from stream (call readFields from Writable). So it's not really needed for it to be neutral element, that's why I left it like this. For aggregate, you mean user implements your version, but still calls regular aggregate? That could be nice, but it require additional creation of aggregated value object in each aggregate call. - Maja ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6134/#review9441 ----------------------------------------------------------- On July 25, 2012, 10:04 a.m., Maja Kabiljo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6134/ > ----------------------------------------------------------- > > (Updated July 25, 2012, 10:04 a.m.) > > > Review request for giraph. > > > Description > ------- > > Now by default all aggregators are reset at the end of each super step. > Aggregators can also be registered as persistent from master, and their value > is aggregated value from all super steps. > > In all compute() executions the value of aggregator from previous super step > is visible, and changes are made on a separate instance, but transparent to > the user. > > I made all the aggregators a bit simpler by creating BasicAggregator which > keeps value and implements get and set, and aggregators for types > (IntAggregator, DoubleAggregator...) which implement createAggregatedValue. > > TestBspBasic.testBspMasterCompute was assuming that vertices see the change > master.compute makes in current super step, that's why the value there is > changed. > > > This addresses bug GIRAPH-259. > https://issues.apache.org/jira/browse/GIRAPH-259 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BasicAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BooleanAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BooleanAndAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BooleanOrAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BooleanOverwriteAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleMaxAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleMinAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleOverwriteAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleProductAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/DoubleSumAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatMaxAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatMinAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatOverwriteAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatProductAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/FloatSumAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntMaxAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntMinAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntOverwriteAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntProductAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/IntSumAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongAggregator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongMaxAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongMinAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongOverwriteAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongProductAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/LongSumAggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/benchmark/RandomMessageBenchmark.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleAggregatorWriter.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMasterComputeVertex.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/examples/VerifyMessage.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/Aggregator.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorUsage.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorWrapper.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorWriter.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceMaster.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/MasterCompute.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/TextAggregatorWriter.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/WorkerContext.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/aggregators/TestBooleanAggregators.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/aggregators/TestDoubleAggregators.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/aggregators/TestFloatAggregators.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/aggregators/TestIntAggregators.java > 1365482 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/aggregators/TestLongAggregators.java > 1365482 > > Diff: https://reviews.apache.org/r/6134/diff/ > > > Testing > ------- > > mvn verify > > > Thanks, > > Maja Kabiljo > >
