> 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? > > Maja Kabiljo wrote: > 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. > > Alessandro Presta wrote: > What do you mean by misinterpret? "Next" makes me think of the next > superstep, whereas "current" makes me think of, ahem, the one that's in > progress :D >
Current is the one that is in progress, but is it the value to be read in it or the value to be written in it? Anyway, I don't have a strong opinion about this, if you think that naming is better I'll change it back. > 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. 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. - 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 > >
