giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alessandro Presta" <alessan...@fb.com>
Subject Re: Review Request: Fixing aggregators
Date Wed, 25 Jul 2012 12:26:13 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6134/#review9441
-----------------------------------------------------------


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.


http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/aggregators/BasicAggregator.java
<https://reviews.apache.org/r/6134/#comment20247>

    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?



http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/Aggregator.java
<https://reviews.apache.org/r/6134/#comment20249>

    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?



http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorUsage.java
<https://reviews.apache.org/r/6134/#comment20248>

    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?



http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/AggregatorWrapper.java
<https://reviews.apache.org/r/6134/#comment20245>

    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?



http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceMaster.java
<https://reviews.apache.org/r/6134/#comment20246>

    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.


- Alessandro Presta


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