Return-Path: X-Original-To: apmail-giraph-dev-archive@www.apache.org Delivered-To: apmail-giraph-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id CF6C5D4C1 for ; Wed, 25 Jul 2012 14:34:25 +0000 (UTC) Received: (qmail 38156 invoked by uid 500); 25 Jul 2012 14:34:25 -0000 Delivered-To: apmail-giraph-dev-archive@giraph.apache.org Received: (qmail 38108 invoked by uid 500); 25 Jul 2012 14:34:25 -0000 Mailing-List: contact dev-help@giraph.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@giraph.apache.org Delivered-To: mailing list dev@giraph.apache.org Delivered-To: moderator for dev@giraph.apache.org Received: (qmail 80252 invoked by uid 99); 25 Jul 2012 12:26:14 -0000 Content-Type: multipart/alternative; boundary="===============5522230742310078269==" MIME-Version: 1.0 Subject: Re: Review Request: Fixing aggregators From: "Alessandro Presta" To: "Maja Kabiljo" , "giraph" , "Alessandro Presta" Date: Wed, 25 Jul 2012 12:26:13 -0000 Message-ID: <20120725122613.4030.50250@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Alessandro Presta" X-ReviewGroup: giraph X-ReviewRequest-URL: https://reviews.apache.org/r/6134/ X-Sender: "Alessandro Presta" References: <20120725100453.4031.53791@reviews.apache.org> In-Reply-To: <20120725100453.4031.53791@reviews.apache.org> Reply-To: "Alessandro Presta" --===============5522230742310078269== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- 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/girap= h/aggregators/BasicAggregator.java I'm most likely missing something, but what prevents us from baking thi= s functionality into Aggregator (thus making it an abstract class, much lik= e Vertex) as opposed to having an interface and an abstract implementation? http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/girap= h/graph/Aggregator.java 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 defin= es an operation and a neutral element, not the internals to change the stat= e. In other words, the user doesn't call get/setValue, that's taken care b= y the infrastructure. = Would this complicate the design? http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/girap= h/graph/AggregatorUsage.java Again, you've probably already thought about this, but just curious: wh= at are the pros/cons of giving the user an instance of Aggregator instead o= f accessing them via their names like we're doing here? http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/girap= h/graph/AggregatorWrapper.java 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/girap= h/graph/BspServiceMaster.java Style: can you format this nicely for future readers? For example, put "Iterables.transform" altogether (you can break after = "=3D" 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. A= ggregators can also be registered as persistent from master, and their valu= e is aggregated value from all super steps. > = > In all compute() executions the value of aggregator from previous super s= tep is visible, and changes are made on a separate instance, but transparen= t to the user. = > = > I made all the aggregators a bit simpler by creating BasicAggregator whic= h keeps value and implements get and set, and aggregators for types (IntAgg= regator, DoubleAggregator...) which implement createAggregatedValue. > = > TestBspBasic.testBspMasterCompute was assuming that vertices see the chan= ge master.compute makes in current super step, that's why the value there i= s 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/g= iraph/aggregators/BasicAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/BooleanAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/BooleanAndAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/BooleanOrAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/BooleanOverwriteAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleMaxAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleMinAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleOverwriteAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleProductAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/DoubleSumAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatMaxAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatMinAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatOverwriteAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatProductAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/FloatSumAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntMaxAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntMinAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntOverwriteAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntProductAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/IntSumAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongAggregator.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongMaxAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongMinAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongOverwriteAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongProductAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/aggregators/LongSumAggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/benchmark/RandomMessageBenchmark.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/examples/SimpleAggregatorWriter.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/examples/SimpleCheckpointVertex.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/examples/SimpleMasterComputeVertex.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/examples/SimplePageRankVertex.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/examples/VerifyMessage.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/Aggregator.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/AggregatorUsage.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/AggregatorWrapper.java PRE-CREATION = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/AggregatorWriter.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/BspService.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/BspServiceMaster.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/BspServiceWorker.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/MasterCompute.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/TextAggregatorWriter.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/Vertex.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/g= iraph/graph/WorkerContext.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/TestBspBasic.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/aggregators/TestBooleanAggregators.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/aggregators/TestDoubleAggregators.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/aggregators/TestFloatAggregators.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/aggregators/TestIntAggregators.java 1365482 = > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/g= iraph/aggregators/TestLongAggregators.java 1365482 = > = > Diff: https://reviews.apache.org/r/6134/diff/ > = > = > Testing > ------- > = > mvn verify > = > = > Thanks, > = > Maja Kabiljo > = > --===============5522230742310078269==--