You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Nitay Joffe <ni...@apache.org> on 2012/10/15 09:18:38 UTC
Review Request: GIRAPH-232: Add metrics system into Giraph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/
-----------------------------------------------------------
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
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Nitay Joffe <ni...@apache.org>.
> On Oct. 16, 2012, 4:45 a.m., Avery Ching wrote:
> > giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java, lines 401-403
> > <https://reviews.apache.org/r/7595/diff/1/?file=176669#file176669line401>
> >
> > This looks unnecessary?
whoops :)
> On Oct. 16, 2012, 4:45 a.m., Avery Ching wrote:
> > giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java, lines 42-55
> > <https://reviews.apache.org/r/7595/diff/1/?file=176672#file176672line42>
> >
> > Minor style thing: Most of the code follows = on the previous line.
sg, we should add checkstyle rules for this so it warns me / others next time?
> On Oct. 16, 2012, 4:45 a.m., Avery Ching wrote:
> > giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java, lines 69-72
> > <https://reviews.apache.org/r/7595/diff/1/?file=176672#file176672line69>
> >
> > Most of the other javadoc comments have a line in between (i.e.
> >
> > /**
> > * Initialize the GiraphMetrics
> > *
> > * @param context Mapper context
> > */
> >
> > Would be great to make this a bit more uniform.
sg, another checkstyle change?
- Nitay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/#review12468
-----------------------------------------------------------
On Oct. 25, 2012, 7:51 a.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7595/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 7:51 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/WorkerClientRequestProcessor.java 210fbd56dbdc01e67e0ba09c64c266ac97c2eae9
> giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228375a7398a6c38134a38bf2055a17a998a204
> giraph/src/main/java/org/apache/giraph/comm/netty/NettyClient.java 3814d12ae42be89a3cf24579a03ba776c03ef06f
> giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java 45dabe1b1fa58035804cce0fee02e882329d1dc6
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 3e659a672baabffd4a5a0824d46f1e705e7354cc
> giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java cb3cb9c8a725830efb2e58abcd995ef27ef2fc80
> giraph/src/main/java/org/apache/giraph/graph/ComputeCallable.java 8602c92ca59c1babfae6bc11059c7dc4f96d4993
> giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java 5092f29d48a03794b2f5d45d7996fad330ae0e40
> giraph/src/main/java/org/apache/giraph/graph/InputSplitsCallable.java e953b11d5e4d07fd0cd82281a89b950af62839ff
> giraph/src/main/java/org/apache/giraph/graph/Vertex.java 4118c3176bd4a9c7d00dc0bd717e591d74886e9c
> giraph/src/main/java/org/apache/giraph/graph/partition/HashWorkerPartitioner.java 9f5bf97fd083190cab7b1a044d7f375ed5feaaad
> 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/MetricGroup.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
>
>
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/#review12468
-----------------------------------------------------------
Hey, this looks pretty good Nitay! Would love to see this finally get in.
I have a few questions:
1) Could you print some sample output or what it looks like when it dumps the metrics?
2) What is the overhead of enabling this? Could you try running a decent app with trunk and then with the this diff (disabled) and enabled? Would like to see the overhead (if any). Say something like PageRankBenchmark 20 workers or so.
I just have a few style suggestions to match the rest of the code style.
giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java
<https://reviews.apache.org/r/7595/#comment26514>
Minor quip: Can you capitalize the comments to match the other ones?
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java
<https://reviews.apache.org/r/7595/#comment26515>
Same here, please Captalize.
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java
<https://reviews.apache.org/r/7595/#comment26519>
setup: Initialized metrics system
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java
<https://reviews.apache.org/r/7595/#comment26520>
This looks unnecessary?
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java
<https://reviews.apache.org/r/7595/#comment26521>
Is this expensive?
giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java
<https://reviews.apache.org/r/7595/#comment26516>
Minor style thing: Most of the code follows = on the previous line.
giraph/src/main/java/org/apache/giraph/metrics/GiraphMetrics.java
<https://reviews.apache.org/r/7595/#comment26517>
Most of the other javadoc comments have a line in between (i.e.
/**
* Initialize the GiraphMetrics
*
* @param context Mapper context
*/
Would be great to make this a bit more uniform.
- Avery Ching
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
>
>
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/#review12936
-----------------------------------------------------------
A suggestion on the naming, perhaps rather than EmptyCounter, EmptyExecutorService, they could be called NoOpCounter and NoOpExecutorService? Not sure if that's clearer or not, but empty had me confused for a bit.
I assume this passed mvn clean install?
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
<https://reviews.apache.org/r/7595/#comment27867>
initGauges
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
<https://reviews.apache.org/r/7595/#comment27842>
gauges
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
<https://reviews.apache.org/r/7595/#comment27844>
You might want to add these methods to GiraphConfiguration
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
<https://reviews.apache.org/r/7595/#comment27843>
worker
giraph/src/main/java/org/apache/giraph/graph/partition/HashWorkerPartitioner.java
<https://reviews.apache.org/r/7595/#comment27846>
What's wrong with the Guava one here?
- Avery Ching
On Oct. 31, 2012, 3:04 a.m., Nitay Joffe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7595/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 3:04 a.m.)
>
>
> Review request for giraph.
>
>
> Description
> -------
>
> GIRAPH-232: Add metrics system into Giraph
>
>
> Diffs
> -----
>
> giraph/pom.xml 655b0f0fc25910be80dda908dcc789a166ad23f1
> 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/WorkerClientRequestProcessor.java 210fbd56dbdc01e67e0ba09c64c266ac97c2eae9
> giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228375a7398a6c38134a38bf2055a17a998a204
> giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java 45dabe1b1fa58035804cce0fee02e882329d1dc6
> giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java d68debc41b44e20c0dcb5907d1a1b398b759b149
> giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 194833c2c0af421de601e2d264e967c026d33d3e
> giraph/src/main/java/org/apache/giraph/graph/ComputeCallable.java 8602c92ca59c1babfae6bc11059c7dc4f96d4993
> giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java f80c54c953ff7ae21e0e2b3cf8610815356d3166
> giraph/src/main/java/org/apache/giraph/graph/InputSplitsCallable.java cf6b1821cad98b8d4bda8b9502df0aad3ad461dd
> giraph/src/main/java/org/apache/giraph/graph/Vertex.java 4118c3176bd4a9c7d00dc0bd717e591d74886e9c
> giraph/src/main/java/org/apache/giraph/graph/partition/HashWorkerPartitioner.java 9f5bf97fd083190cab7b1a044d7f375ed5feaaad
> 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/MetricGroup.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/metrics/package-info.java PRE-CREATION
> giraph/src/main/java/org/apache/giraph/utils/MemoryUtils.java 8fc403201b784bd4dab733d6764d1cf7ed0295a6
> pom.xml c0842192a1629ecf98273c51d7185295da52f9cd
>
> Diff: https://reviews.apache.org/r/7595/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nitay Joffe
>
>
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Nitay Joffe <ni...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/
-----------------------------------------------------------
(Updated Oct. 31, 2012, 3:04 a.m.)
Review request for giraph.
Description
-------
GIRAPH-232: Add metrics system into Giraph
Diffs (updated)
-----
giraph/pom.xml 655b0f0fc25910be80dda908dcc789a166ad23f1
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/WorkerClientRequestProcessor.java 210fbd56dbdc01e67e0ba09c64c266ac97c2eae9
giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228375a7398a6c38134a38bf2055a17a998a204
giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java 45dabe1b1fa58035804cce0fee02e882329d1dc6
giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java d68debc41b44e20c0dcb5907d1a1b398b759b149
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 194833c2c0af421de601e2d264e967c026d33d3e
giraph/src/main/java/org/apache/giraph/graph/ComputeCallable.java 8602c92ca59c1babfae6bc11059c7dc4f96d4993
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java f80c54c953ff7ae21e0e2b3cf8610815356d3166
giraph/src/main/java/org/apache/giraph/graph/InputSplitsCallable.java cf6b1821cad98b8d4bda8b9502df0aad3ad461dd
giraph/src/main/java/org/apache/giraph/graph/Vertex.java 4118c3176bd4a9c7d00dc0bd717e591d74886e9c
giraph/src/main/java/org/apache/giraph/graph/partition/HashWorkerPartitioner.java 9f5bf97fd083190cab7b1a044d7f375ed5feaaad
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/MetricGroup.java PRE-CREATION
giraph/src/main/java/org/apache/giraph/metrics/package-info.java PRE-CREATION
giraph/src/main/java/org/apache/giraph/utils/MemoryUtils.java 8fc403201b784bd4dab733d6764d1cf7ed0295a6
pom.xml c0842192a1629ecf98273c51d7185295da52f9cd
Diff: https://reviews.apache.org/r/7595/diff/
Testing
-------
Thanks,
Nitay Joffe
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Nitay Joffe <ni...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/
-----------------------------------------------------------
(Updated Oct. 25, 2012, 7:51 a.m.)
Review request for giraph.
Changes
-------
Put in all the requested changes.
I tested this using: org.apache.giraph.benchmark.PageRankBenchmark -e 3 -s 5 -V 20000000 -w 20
It seems to have little overhead. The wall clock time is around 45K msec and the CPU time is 3.5M msec.
I did a few runs of each configuration: without the diff, with the diff but metrics turned off, and with the diff metrics turned on.
There was not a substantial increase with the metrics turned on.
I changed the groupings of the metrics. Namely I put them into categories: compute, io, network, system, and user.
You can see the new output here: https://gist.github.com/7b88a70824f8f4d88b74
I also fixed the JMX. It works out the box by default _when_ you are using the
static Metrics provided by the library. Since we toggle between our own
Register and an empty one, I needed to add my own JmxReporter. Anyways, it
works now. Check it out: http://imgur.com/xqHE5
Let me know what you guys think.
Description
-------
GIRAPH-232: Add metrics system into Giraph
Diffs (updated)
-----
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/WorkerClientRequestProcessor.java 210fbd56dbdc01e67e0ba09c64c266ac97c2eae9
giraph/src/main/java/org/apache/giraph/comm/netty/ByteCounter.java f228375a7398a6c38134a38bf2055a17a998a204
giraph/src/main/java/org/apache/giraph/comm/netty/NettyClient.java 3814d12ae42be89a3cf24579a03ba776c03ef06f
giraph/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java 45dabe1b1fa58035804cce0fee02e882329d1dc6
giraph/src/main/java/org/apache/giraph/graph/BspServiceMaster.java 3e659a672baabffd4a5a0824d46f1e705e7354cc
giraph/src/main/java/org/apache/giraph/graph/BspServiceWorker.java cb3cb9c8a725830efb2e58abcd995ef27ef2fc80
giraph/src/main/java/org/apache/giraph/graph/ComputeCallable.java 8602c92ca59c1babfae6bc11059c7dc4f96d4993
giraph/src/main/java/org/apache/giraph/graph/GraphMapper.java 5092f29d48a03794b2f5d45d7996fad330ae0e40
giraph/src/main/java/org/apache/giraph/graph/InputSplitsCallable.java e953b11d5e4d07fd0cd82281a89b950af62839ff
giraph/src/main/java/org/apache/giraph/graph/Vertex.java 4118c3176bd4a9c7d00dc0bd717e591d74886e9c
giraph/src/main/java/org/apache/giraph/graph/partition/HashWorkerPartitioner.java 9f5bf97fd083190cab7b1a044d7f375ed5feaaad
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/MetricGroup.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
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Eli Reisman <ap...@gmail.com>.
Sounds great!
On Mon, Oct 15, 2012 at 1:50 PM, Nitay Joffe <ni...@apache.org> wrote:
>
>
> > On Oct. 15, 2012, 5:59 p.m., Eli Reisman wrote:
> > > 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.
>
> There is no metric server / graphite piece. I actually ripped that piece
> out (but can put it back in). The every 90 seconds is printing to console,
> which is completely configurable (can turn it off all together).
> The Empty* classes are in case you wish to turn off metrics all together.
> They are dummy classes so that the user code stays the same while having no
> effect.
> Does that make sense?
>
>
> - Nitay
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7595/#review12444
> -----------------------------------------------------------
>
>
> 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
> >
> >
>
>
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Nitay Joffe <ni...@apache.org>.
> On Oct. 15, 2012, 5:59 p.m., Eli Reisman wrote:
> > 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.
There is no metric server / graphite piece. I actually ripped that piece out (but can put it back in). The every 90 seconds is printing to console, which is completely configurable (can turn it off all together).
The Empty* classes are in case you wish to turn off metrics all together. They are dummy classes so that the user code stays the same while having no effect.
Does that make sense?
- Nitay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7595/#review12444
-----------------------------------------------------------
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
>
>
Re: Review Request: GIRAPH-232: Add metrics system into Giraph
Posted by Eli Reisman <in...@gmail.com>.
-----------------------------------------------------------
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
>
>