giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maja Kabiljo" <majakabi...@fb.com>
Subject Re: Review Request: Fixing aggregators
Date Wed, 25 Jul 2012 13:16:55 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message