giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eli Reisman" <initialcont...@gmail.com>
Subject Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Date Mon, 15 Oct 2012 17:59:06 GMT

-----------------------------------------------------------
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 aside 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 sufficient 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 report
is good but even then I found when debugging or analyzing runs for best results we had to
add metrics objects to a build of trunk, then remove again once we had the data we needed.

Also, I'm a bit fuzzy on the implementation of the EmptyRegistry and metrics -- 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 receiving metrics
server and/or configure and add metrics objects to the Giraph source for those who want to
use this feature. I think if people adopt this it will really accelerate development and testing
for Giraph contributors accross the board.

Again, thanks so much for doing this, I was considering suggesting taking over from Jakob
myself but I figured he'd get around to this when he had time.

- 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-CREATION 
>   giraph/src/main/java/com/yammer/metrics/core/EmptyExecutorService.java PRE-CREATION

>   giraph/src/main/java/com/yammer/metrics/core/EmptyHistogram.java PRE-CREATION 
>   giraph/src/main/java/com/yammer/metrics/core/EmptyMeter.java PRE-CREATION 
>   giraph/src/main/java/com/yammer/metrics/core/EmptyTimer.java PRE-CREATION 
>   giraph/src/main/java/com/yammer/metrics/core/NullGuage.java PRE-CREATION 
>   giraph/src/main/java/com/yammer/metrics/core/package-info.java PRE-CREATION 
>   giraph/src/main/java/org/apache/giraph/comm/WorkerClient.java efef8f69de4ff39e508e6e2141e1e14bfff70c37

>   giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228375a7398a6c38134a38bf2055a17a998a204

>   giraph/src/main/java/org/apache/giraph/comm/netty/NettyClient.java 9e541f3a37fe71174d42aa2ee905d2a9e6b8a65c

>   giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClient.java 6ea20bc8b2d2c0f9d727406eff88ce35cbc139f0

>   giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientServer.java 6600e78c2856befe5e67bf843a7028b493ede694

>   giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java ade33a1d7320e9ed381f1fe62213de7d4ce8c3a1

>   giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java ff59f40eb3b7e7008bf578a57be22e8996540f8b

>   giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java e9217f7689b72008075805e7f504e00bf06f7df1

>   giraph/src/main/java/org/apache/giraph/graph/Vertex.java cd78810344fdb3fdf29f5c471efb97c4de75c818

>   giraph/src/main/java/org/apache/giraph/metrics/EmptyMetricsRegistry.java PRE-CREATION

>   giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java PRE-CREATION 
>   giraph/src/main/java/org/apache/giraph/metrics/package-info.java PRE-CREATION 
>   giraph/src/main/java/org/apache/giraph/utils/GiraphTimer.java PRE-CREATION 
>   giraph/src/main/java/org/apache/giraph/utils/MemoryUtils.java 8fc403201b784bd4dab733d6764d1cf7ed0295a6

>   giraph/src/main/java/org/apache/giraph/utils/MilliSecTimer.java PRE-CREATION 
>   pom.xml 1fd2791bf42df465f532fbdacfd1979e6b4b3857 
> 
> Diff: https://reviews.apache.org/r/7595/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Nitay Joffe
> 
>


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