Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt I think this is ready to merge. Just waiting on your +1.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt @afine Do you think this PR is ready to committed before the
Dropwizard one?
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt I asked around quickly:
HBase - own library (looks like they've started to use dropwizard somehow)
HDFS - Hadoop Metrics2 (included in hadoop-common)
Kafka - Yammer (dropwizard
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/415
> One approach (which this PR tries to follow) is to expose basic values
which can be sampled by a more sophisticated monitoring tool. Average, sliding
window, etc. can be implemented in there.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/415
> Do you think it's worth merging this PR and refactor it in a separate one
to use that lib?
I'd say, let's consider it separately. There's probably some work to do to
identify what we
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt I added proposal stats to 'mntr' command output as requested.
In the meantime I found the following metrics lib which could be useful and
easily integrated into Zookeeper:
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt That's a perfectly valid point Pat, I've been asked about it several
times before. Therefore I added some more comments to the parent Jira of
monitoring, please take a look at the
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/415
This seems like a reasonable addition (see below). I like the refactorings
(kudos). The code seems fine. I do have a couple questions though.
1) this information should also be added to
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt @afine are u happy with the change?
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt do you think we can commit this patch?
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@afine @phunt Findbugs issues have been resolved. Please review & commit.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@afine You're not missing anything, I didn't add jetty support to this
patch intentionally, because afaik jetty is not supported in 3.4. So the plan
is to keep this patch compatible with all
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt 4lw changes have been added.
@afine please start the review over, because so many things have been
changed since you approved it.
For some weird reason I cannot get a green
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt Sorry, I've completely forgotten about it.
I believe the easiest would be to add proposal stats to 'stat' 4lw in this
patch, so that it could be easily applied to all major branches.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/415
What's the plan re my other comment
> We try to keep jmx, 4lw and jetty interfaces (new in 3.5) in lock step
wrt the metrics they provide.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt I've added capability of resetting proposal statistics as well as
unit tests for LeaderBean class.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@phunt Thanks for looking into this. I'll include the reset capability into
this patch as suggested.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/415
I didn't look at this in much depth yet but one obvious things struck me -
you are missing the reset capability. See how we handle latency tracking in the
beans.
We try to keep jmx, 4lw
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@afine Thanks for approving.
I've added the same byte array verification to the other unit test too.
Also squashed everything into single commit.
Ready to merge.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/415
@afine Take a look at please the latest commit, I've added verification of
the byte array itself to serializeRequest() unit test. I'm not 100% sure it's
needed, but it might makes sense this
20 matches
Mail list logo
