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 14:30:43 GMT


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


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