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 D812DD1EE for ; Mon, 15 Oct 2012 17:59:09 +0000 (UTC) Received: (qmail 5343 invoked by uid 500); 15 Oct 2012 17:59:09 -0000 Delivered-To: apmail-giraph-dev-archive@giraph.apache.org Received: (qmail 5285 invoked by uid 500); 15 Oct 2012 17:59:08 -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 Received: (qmail 5270 invoked by uid 99); 15 Oct 2012 17:59:08 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 15 Oct 2012 17:59:08 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id B4DDC1C44DF; Mon, 15 Oct 2012 17:59:06 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============6109321238533691567==" MIME-Version: 1.0 Subject: Re: Review Request: GIRAPH-232: Add metrics system into Giraph From: "Eli Reisman" To: "Eli Reisman" , "giraph" , "Nitay Joffe" Date: Mon, 15 Oct 2012 17:59:06 -0000 Message-ID: <20121015175906.16968.27812@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Eli Reisman" X-ReviewGroup: giraph X-ReviewRequest-URL: https://reviews.apache.org/r/7595/ X-Sender: "Eli Reisman" References: <20121015071838.16939.78996@reviews.apache.org> In-Reply-To: <20121015071838.16939.78996@reviews.apache.org> Reply-To: "Eli Reisman" --===============6109321238533691567== 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/7595/#review12444 ----------------------------------------------------------- Hi Nitay, Great job, I got to use this patch for a time before Jakob had to set it as= ide and it is incredibly helpful. A couple questions/concerns: Be careful about hardcoding any metrics objects into the codebase, usually = the calls to the metrics package can delay processing and given a job of su= fficient size, even periodic messaging from a bunch of worker tasks to the = graphite server picking up the metrics data will overwhelm that server and = it will lose data or connections. Your default of 90 seconds per metrics re= port is good but even then I found when debugging or analyzing runs for bes= t results we had to add metrics objects to a build of trunk, then remove ag= ain once we had the data we needed. Also, I'm a bit fuzzy on the implementation of the EmptyRegistry and metric= s -- is each singleton instance (like the EmptyCounter) simply managing all= Counter objects for that JVM/class loader's metric instances and updating = all of them by name parameter? How are we using a singleton here? You might want to pair this with a quick wiki page on how to set up the rec= eiving metrics server and/or configure and add metrics objects to the Girap= h source for those who want to use this feature. I think if people adopt th= is it will really accelerate development and testing for Giraph contributor= s accross the board. Again, thanks so much for doing this, I was considering suggesting taking o= ver from Jakob myself but I figured he'd get around to this when he had tim= e. - Eli Reisman On Oct. 15, 2012, 7:18 a.m., Nitay Joffe wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7595/ > ----------------------------------------------------------- > = > (Updated Oct. 15, 2012, 7:18 a.m.) > = > = > Review request for giraph. > = > = > Description > ------- > = > GIRAPH-232: Add metrics system into Giraph > = > = > Diffs > ----- > = > giraph/pom.xml 31da7b45d90104459be07eefe28d59f71fd42c3b = > giraph/src/main/java/com/yammer/metrics/core/EmptyCounter.java PRE-CREA= TION = > giraph/src/main/java/com/yammer/metrics/core/EmptyExecutorService.java = PRE-CREATION = > giraph/src/main/java/com/yammer/metrics/core/EmptyHistogram.java PRE-CR= EATION = > giraph/src/main/java/com/yammer/metrics/core/EmptyMeter.java PRE-CREATI= ON = > giraph/src/main/java/com/yammer/metrics/core/EmptyTimer.java PRE-CREATI= ON = > giraph/src/main/java/com/yammer/metrics/core/NullGuage.java PRE-CREATIO= N = > giraph/src/main/java/com/yammer/metrics/core/package-info.java PRE-CREA= TION = > giraph/src/main/java/org/apache/giraph/comm/WorkerClient.java efef8f69d= e4ff39e508e6e2141e1e14bfff70c37 = > giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228= 375a7398a6c38134a38bf2055a17a998a204 = > giraph/src/main/java/org/apache/giraph/comm/netty/NettyClient.java 9e54= 1f3a37fe71174d42aa2ee905d2a9e6b8a65c = > giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClient.jav= a 6ea20bc8b2d2c0f9d727406eff88ce35cbc139f0 = > giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientServ= er.java 6600e78c2856befe5e67bf843a7028b493ede694 = > giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java ade3= 3a1d7320e9ed381f1fe62213de7d4ce8c3a1 = > giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java ff59= f40eb3b7e7008bf578a57be22e8996540f8b = > giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java e9217f768= 9b72008075805e7f504e00bf06f7df1 = > giraph/src/main/java/org/apache/giraph/graph/Vertex.java cd78810344fdb3= fdf29f5c471efb97c4de75c818 = > giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.jav= a PRE-CREATION = > giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java PRE-C= REATION = > giraph/src/main/java/org/apache/giraph/metrics/package-info.java PRE-CR= EATION = > giraph/src/main/java/org/apache/giraph/utils/GiraphTimer.java PRE-CREAT= ION = > giraph/src/main/java/org/apache/giraph/utils/MemoryUtils.java 8fc403201= b784bd4dab733d6764d1cf7ed0295a6 = > giraph/src/main/java/org/apache/giraph/utils/MilliSecTimer.java PRE-CRE= ATION = > pom.xml 1fd2791bf42df465f532fbdacfd1979e6b4b3857 = > = > Diff: https://reviews.apache.org/r/7595/diff/ > = > = > Testing > ------- > = > = > Thanks, > = > Nitay Joffe > = > --===============6109321238533691567==--