> 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? > > Maja Kabiljo wrote: > 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. > > Alessandro Presta wrote: > Got it, it would complicate the design. > What I'm thinking is that it's a bit confusing for the user to define > aggregate(A) for its Aggregator, and then call aggregate(String, A) in its > AggregatorUsage. > But if there is no simple way to do it, I'm ok with this. > > Maja Kabiljo wrote: > Before this change user was calling getAggregator(String) and then > aggregate(A) on that aggregator, which is basically the same thing. > AggregatorUsage is not related to a single aggregator, it manages all the > aggregators, so calling just aggregate(A) there wouldn't make sense. If it's > just a naming issue, propose a better name and I'll make the change.
I didn't mean to call aggregate(A) as a method of AggregatorUsage, but of a specific aggregator instance. Anyway we concluded that this would be messy to implement, so never mind. If it has to be a method of AggregatorUsage, I can't think of a different name that makes sense. I guess this is ok. - Alessandro ----------------------------------------------------------- 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 > >
