[Zookeeper-developer] [ zookeeper-Patches-1913998 ] JMX instrumentation

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Patches item #1913998, was opened at 2008-03-14 01:52
Message generated for change (Comment added) made by mahadevkonar
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: server
Group: 3.0.0
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Andrew Kornev (akornev)
>Assigned to: Benjamin Reed (breed)
Summary: JMX instrumentation

Initial Comment:
This patch adds support for management and monitoring via JMX. It uses the MXBean feature made available in Java 6.

The instrumentation code relies on the new lifecycle event notification mechanism to dynamically register and unregister MBeans. 

The ant build file has NOT been modified to build this new feature yet. There are no javadocs for the new classes either (but this will be fixed, of course).

----------------------------------------------------------------------

>Comment By: Mahadev Konar (mahadevkonar)
Date: 2008-05-29 20:14

Message:
Logged In: YES 
user_id=1926680
Originator: NO

assigning to ben since he is reviewing the code.

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-05-09 18:34

Message:
Logged In: YES 
user_id=1926652
Originator: YES

Fixed compilation errors introduced as a result of [1956480] patch. Now is
really a good time to start reviewing this patch.
File Added: jmx-4.patch

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-05-07 18:25

Message:
Logged In: YES 
user_id=1926652
Originator: YES

Moved around ManagedZooKeeperServer and ManagedQuorumPeer classes back to
the jmx source tree to make it possible to build the src source tree
whithout the jmx tree. Now would be a good time to start reviewing this
patch.
File Added: jmx-3.patch

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-05-02 01:06

Message:
Logged In: YES 
user_id=1926652
Originator: YES

Added javadocs, updated the code to use log4j, moved common classes to the
src/ directory tree.
File Added: jmx-2.patch

----------------------------------------------------------------------

Comment By: Mahadev Konar (mahadevkonar)
Date: 2008-03-17 20:44

Message:
Logged In: YES 
user_id=1926680
Originator: NO

that is primarily my concern. Meaning if jmx is to be enabled in
production then creating an ojbect per connection would deteriorate the
scalability of zookeeper. What I mean is that do we actually need per
connection management in jmx? or we could have just connection stats in jmx
and some other means to debug problems with connections (other than jmx)? 

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-03-17 20:14

Message:
Logged In: YES 
user_id=1926652
Originator: YES

JMX is primarily for management and monitoring, and my hope is to have JMX
enabled in production by default. JMX instrumentation code will eventually
rely on the /proc data tree (when it becomes available) to get
connection-related information. But we're still gonna have to have a bean
per connection registered with MBeanServer. Dynamic
registering/unregistering of connection beans may indeed incur some
(insignificant) overhead, but I'm not aware of any other way of making a
dynamic resource avilable to JMX.

----------------------------------------------------------------------

Comment By: Mahadev Konar (mahadevkonar)
Date: 2008-03-17 19:50

Message:
Logged In: YES 
user_id=1926680
Originator: NO

3) for this what I meant was that .. we c annot run with jmx on in
production .. and if connection tracking is for debugging purpose then jmx
just does not sound the right tool for debugging... 

4) thats true... but lets have a better process in line :) ... sorry to
have started with you.

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-03-17 19:23

Message:
Logged In: YES 
user_id=1926652
Originator: YES

1) yes!
2) updated the code to use logError() for logging errors only and
logTraceMessage() for logging debug/info.
3) not sure I understand the concern. Connection tracking using /proc FS
is orthogonal to JMX. JMX Beans would have to be instantiated for each new
connection anyway if we want to be able to manage connections thru JMX.
4) this may take a while. Historically, however, lacking and/or misleading
javadocs have never been a showstopper for the existing ZK code ;-)

----------------------------------------------------------------------

Comment By: Mahadev Konar (mahadevkonar)
Date: 2008-03-15 18:02

Message:
Logged In: YES 
user_id=1926680
Originator: NO

1) 

dataTreeBean = new com.yahoo.zookeeper.jmx.server.DataTreeBean(
+                        server.dataTree);

can we just replace this with DataTreeBean(datatree)?

2) also i wonder if we should have logging levels on the server side like
debug and info , warn, fatal

right now we just have 

logerror and logwarn... 

and we end up using logError for everything .. 

3) am really unconfortable with the idea that we keep track of all the
connections using jmx beans? I thought we were going to put all this info
in the /proc file system? We end up creating a new object connection bean
for every connection (as per my understanding) ... 

4) can you upload a patch with javadocs? :) Its easier to review :).

----------------------------------------------------------------------

Comment By: Andrew Kornev (akornev)
Date: 2008-03-14 17:11

Message:
Logged In: YES 
user_id=1926652
Originator: YES

Are you sure it's this patch that has tabs in it? Or, maybe, you meant the
other one (the refactoring patch)? Because that one does have tabs (which
I've already fixed in locally). But this one does not. Thanks for
reviewing!

----------------------------------------------------------------------

Comment By: Mahadev Konar (mahadevkonar)
Date: 2008-03-14 02:20

Message:
Logged In: YES 
user_id=1926680
Originator: NO

this patch has tabs in it... i havent finished reviewing... just getting
my review comments out... ill let you know when I am done fully reviewing
the patch so that you dont have to upload it again and again.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147