> 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? > > Maja Kabiljo wrote: > 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.
Then we can probably merge them for now. We can always go back if need arises, but I like to avoided unneeded complexity. > 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? > > Maja Kabiljo wrote: > 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. On createAggregatedValue: got it, although it's more of a naming issue than anything. I guess having a separate reset() avoids creating new instances in case the value type is huge, but when does that happen in practice? What I'm saying is: maybe we can use the same method both for instantiating a fresh value and for resetting. One less thing to implement. No big deal anyway. On aggregate: yeah sorry, I should have used another name. I see what you're saying. I usually consider this sort of thing a premature optimization, are there use cases when the aggregator value is some big object like a HashMap? For Writable wrappers around numbers, it wouldn't probably be a big issue. > 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. 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. > 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. 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 - 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 > >
