Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-5084

Cassandra should expose connected client state via JMX

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Fix Version/s: 1.2.11
    • Component/s: None
    • Labels:

      Description

      There is currently no good way to determine or estimate how many clients are connected to a cassandra node without using netstat or (if using sync thrift server) counting threads. There is also no way to understand what state any given connection is in. People regularly come into #cassandra/cassandra-user@ and ask how to get the equivalent of a MySQL "SHOW FULL PROCESSLIST."

      While I understand that feature parity with SHOW FULL PROCESSLIST/information_schema.processlist is unlikely, even a few basic metrics like "number of connected clients" or "number of active clients" would greatly help with this operational information need.

      1. trunk-5084.patch
        6 kB
        Suresh
      2. trunk-5084-native.patch
        6 kB
        Mikhail Stepura
      3. trunk-5084-sept4.patch
        7 kB
        Suresh
      4. 5084-v1.txt
        6 kB
        Brandon Williams
      5. cassandra-1.2-5084-native.patch
        6 kB
        Mikhail Stepura
      6. cassandra-1.2-5084-metrics.patch
        4 kB
        Mikhail Stepura
      7. 5084_thrift_V2.patch
        5 kB
        Suresh
      8. cassandra-1.2-5084-metrics-v2.patch
        7 kB
        Mikhail Stepura

        Activity

        Hide
        jbellis Jonathan Ellis added a comment -

        committed!

        Show
        jbellis Jonathan Ellis added a comment - committed!
        Hide
        mishail Mikhail Stepura added a comment -

        New version of the patch

        • Singleton instance of ClientMetrics object
        • Each server registers its connections counter in that instance

        Also added a comment for a logic around allChannels.size()

        Show
        mishail Mikhail Stepura added a comment - New version of the patch Singleton instance of ClientMetrics object Each server registers its connections counter in that instance Also added a comment for a logic around allChannels.size()
        Hide
        jbellis Jonathan Ellis added a comment -

        Looks good overall. Nits:

        • Probably makes more sense to instantiate ClientMetrics in StorageService or as a singleton than in the native Server (which is still optional).
        • I'd shorten the names to connectedNativeClients / connectedThriftClients
        • Brace on newline, please
        • What's up with "-1 for bootstrap" on the native server side? Apologies if this was addressed above already.
        Show
        jbellis Jonathan Ellis added a comment - Looks good overall. Nits: Probably makes more sense to instantiate ClientMetrics in StorageService or as a singleton than in the native Server (which is still optional). I'd shorten the names to connectedNativeClients / connectedThriftClients Brace on newline, please What's up with "-1 for bootstrap" on the native server side? Apologies if this was addressed above already.
        Hide
        sureshsajja Suresh added a comment - - edited

        Modified patch, for connection counts and session details for thrift session, is attached here

        Show
        sureshsajja Suresh added a comment - - edited Modified patch, for connection counts and session details for thrift session, is attached here
        Hide
        mishail Mikhail Stepura added a comment -

        attached the patch which combines counters for both Thrift (kudos Suresh) and Native protocols and exposes them via Codahale Metrics

        Show
        mishail Mikhail Stepura added a comment - attached the patch which combines counters for both Thrift (kudos Suresh ) and Native protocols and exposes them via Codahale Metrics
        Hide
        sureshsajja Suresh added a comment -

        Yes, I am working on it.

        I will modify the patch to support only session counts?

        Show
        sureshsajja Suresh added a comment - Yes, I am working on it. I will modify the patch to support only session counts?
        Hide
        jbellis Jonathan Ellis added a comment -

        Are you still working on this Suresh?

        Show
        jbellis Jonathan Ellis added a comment - Are you still working on this Suresh ?
        Hide
        iamaleksey Aleksey Yeschenko added a comment -

        I 100% agree with Dave here. Exposing the current clientstate keyspace is definitely useless, at least in CQL3 world, where you can specify keyspace.table in almost every statement. And user login information is mostly useless as well.

        Just the session counts, as originally suggested in the issue description, will do.

        Show
        iamaleksey Aleksey Yeschenko added a comment - I 100% agree with Dave here. Exposing the current clientstate keyspace is definitely useless, at least in CQL3 world, where you can specify keyspace.table in almost every statement. And user login information is mostly useless as well. Just the session counts, as originally suggested in the issue description, will do.
        Hide
        dbrosius Dave Brosius added a comment -

        I think we should limit this patch for now to exposing just connection counts both thrift/native, and push to a separate patch the idea of exposing user information and keyspace per session information. Given a pooled environment, i'm not sure that this information makes a whole lot of sense, as it's not usually front end users, but a common shared 'db user'. Similarly with keyspace info.

        Show
        dbrosius Dave Brosius added a comment - I think we should limit this patch for now to exposing just connection counts both thrift/native, and push to a separate patch the idea of exposing user information and keyspace per session information. Given a pooled environment, i'm not sure that this information makes a whole lot of sense, as it's not usually front end users, but a common shared 'db user'. Similarly with keyspace info.
        Hide
        mishail Mikhail Stepura added a comment -

        Changes for the native protocol tracker on top of 1.2

        • Attach a Connection to a Channel in addConnection method. It should be safe to do that in 1.2, because the Connection is already constructed. However, it's unnecessary (and even may be unsafe) to do it in the trunk, where Connection is registered in its constructor.
        • Renamed MBean to org.apache.cassandra.Client:type=NativeSessionManager
        • Renamed the attribute to NumOfConnectedClients
        • Renamed the operation to showConnectionDetailsForAllClients
        Show
        mishail Mikhail Stepura added a comment - Changes for the native protocol tracker on top of 1.2 Attach a Connection to a Channel in addConnection method. It should be safe to do that in 1.2, because the Connection is already constructed. However, it's unnecessary (and even may be unsafe) to do it in the trunk, where Connection is registered in its constructor. Renamed MBean to org.apache.cassandra.Client:type=NativeSessionManager Renamed the attribute to NumOfConnectedClients Renamed the operation to showConnectionDetailsForAllClients
        Hide
        mishail Mikhail Stepura added a comment -

        Brandon Williams There is a difference between 1.2 and trunk in the way Cassandra handles Netty's Channels/Connections. Basically a connection is not attached to a channel (as an attachment), so I need some time to figure out the proper/safest way to port the changes to 1.2.

        Show
        mishail Mikhail Stepura added a comment - Brandon Williams There is a difference between 1.2 and trunk in the way Cassandra handles Netty's Channels/Connections. Basically a connection is not attached to a channel (as an attachment), so I need some time to figure out the proper/safest way to port the changes to 1.2.
        Hide
        jbellis Jonathan Ellis added a comment -

        How does that look Dave Brosius?

        Show
        jbellis Jonathan Ellis added a comment - How does that look Dave Brosius ?
        Hide
        brandon.williams Brandon Williams added a comment -

        Oops, nevermind, that was probably the result of my attempt to backport it hastily. Eyeballing the patch I see it now.

        Show
        brandon.williams Brandon Williams added a comment - Oops, nevermind, that was probably the result of my attempt to backport it hastily. Eyeballing the patch I see it now.
        Hide
        mishail Mikhail Stepura added a comment -

        Brandon Williams I'll try to attach a new patch today. Could you please also explain "You also don't actually implement an MBean" thing. I tested the changes locally and saw no errors.

        Show
        mishail Mikhail Stepura added a comment - Brandon Williams I'll try to attach a new patch today. Could you please also explain "You also don't actually implement an MBean" thing. I tested the changes locally and saw no errors.
        Hide
        brandon.williams Brandon Williams added a comment -

        Mikhail Stepura could you rebase to 1.2, and try to make the namespace/methods as similar to the thrift side as possible? You also don't actually implement an MBean, which makes javax very angry.

        Show
        brandon.williams Brandon Williams added a comment - Mikhail Stepura could you rebase to 1.2, and try to make the namespace/methods as similar to the thrift side as possible? You also don't actually implement an MBean, which makes javax very angry.
        Hide
        brandon.williams Brandon Williams added a comment -

        I don't see any reason to wait for 2.1 to expose this, so here's a rebase of Suresh's patch for 1.2, with a few minor changes.

        Show
        brandon.williams Brandon Williams added a comment - I don't see any reason to wait for 2.1 to expose this, so here's a rebase of Suresh's patch for 1.2, with a few minor changes.
        Hide
        sureshsajja Suresh added a comment -

        Hi Dave, Brandon, Robert, Jonathan,

        Thank you for reviewing this.

        I added no-op method for initialization.
        Edited typo in operation name as NumOfConnectedClients

        Attached patch.

        Show
        sureshsajja Suresh added a comment - Hi Dave, Brandon, Robert, Jonathan, Thank you for reviewing this. I added no-op method for initialization. Edited typo in operation name as NumOfConnectedClients Attached patch.
        Hide
        mishail Mikhail Stepura added a comment -

        Patch for the native interface. Exported ConnectionsCount attribute and listConnections operation via JMX

        Show
        mishail Mikhail Stepura added a comment - Patch for the native interface. Exported ConnectionsCount attribute and listConnections operation via JMX
        Hide
        dbrosius Dave Brosius added a comment - - edited

        it's just there to let the classloader do it's thing. stuff gets registered in the static block.

        could do a myriad of things,

        Class.forName(ThriftSessionManager.class.getName());
        or
        ThriftSessionManager t = ThriftSessionManager.instance;
        etc, etc

        adding a no-op ThriftSessionManager.install() method in there, and calling it would be more self documenting. i suppose.

        Show
        dbrosius Dave Brosius added a comment - - edited it's just there to let the classloader do it's thing. stuff gets registered in the static block. could do a myriad of things, Class.forName(ThriftSessionManager.class.getName()); or ThriftSessionManager t = ThriftSessionManager.instance; etc, etc adding a no-op ThriftSessionManager.install() method in there, and calling it would be more self documenting. i suppose.
        Hide
        jbellis Jonathan Ellis added a comment -

        Do you have to invoke a method or does just touching .instance work? (Either way a comment would be nice.)

        Show
        jbellis Jonathan Ellis added a comment - Do you have to invoke a method or does just touching .instance work? (Either way a comment would be nice.)
        Hide
        dbrosius Dave Brosius added a comment -

        yes

        Show
        dbrosius Dave Brosius added a comment - yes
        Hide
        brandon.williams Brandon Williams added a comment -

        Are you calling ThriftSessionManager.instance.hashCode(); in CassandraServer just as a way of initializing it?

        Show
        brandon.williams Brandon Williams added a comment - Are you calling ThriftSessionManager.instance.hashCode(); in CassandraServer just as a way of initializing it?
        Hide
        jbellis Jonathan Ellis added a comment -

        (Marking Patch Submitted)

        Show
        jbellis Jonathan Ellis added a comment - (Marking Patch Submitted)
        Hide
        rcoli Robert Coli added a comment -

        Suresh : first, thanks for taking up this ticket! great work so far!

        minor quibble : NoOfConnectedClients and other 'No' using methods seems oddly named.. why 'No' and not 'Num' or 'Number'?

        Show
        rcoli Robert Coli added a comment - Suresh : first, thanks for taking up this ticket! great work so far! minor quibble : NoOfConnectedClients and other 'No' using methods seems oddly named.. why 'No' and not 'Num' or 'Number'?
        Hide
        sureshsajja Suresh added a comment -

        For JMX name, I could think as org.apache.cassandra.Client:type=ClientSessionManager.

        I meant to expose the number of connected clients and their socket details. This number include idle/active connections. It makes sense to rename the attributes as NoOfConnectedClients, SocketDetailsForAllClients.

        I think there will not be any concern exposing user Yes, probably we can extend this Mbean for other operations(kill specific users by admin) later

        Other programing concerns are addressed.

        Show
        sureshsajja Suresh added a comment - For JMX name, I could think as org.apache.cassandra.Client:type=ClientSessionManager. I meant to expose the number of connected clients and their socket details. This number include idle/active connections. It makes sense to rename the attributes as NoOfConnectedClients, SocketDetailsForAllClients. I think there will not be any concern exposing user Yes, probably we can extend this Mbean for other operations(kill specific users by admin) later Other programing concerns are addressed.
        Hide
        dbrosius Dave Brosius added a comment -

        Couple of points of note:
        This patch doesn't interact with the native interface, which going forward in trunk would seem to be more relevant. At the least that probably means the JMX name shouldn't include 'thrift'. connections? something else.

        Rather than returning error strings, probably would be better to just throw RuntimeExceptions, as programmatically you wouldn't be able to tell that "No active connection found for given host and port" wasn't a user name. Sure it looks fine in jconsole, but i'm talking about a programatic access.

        Creating a ThriftSessionManager field in CassandraServer doesn't seem necessary just to make sure that it is loaded first. One would think you could just do something like
        ThriftSessionManager.instance.hashCode() in the CassandraServer ctor.

        When i saw the word 'Active' in the jmx names, i immediately assumed you were talking about how many clients had code running for them in the server at the present time. I'm not sure if there's a better word for what you are reporting or not. In addition, with connection pooling, these numbers maybe larger than expected, so is that statistic of use? Or do you really want active use connections?

        Is there any concern about exposing user identity thru jmx? probably not, just asking.

        As soon as you do expose this however, admins will want the ability to kill a specific users connection

        small nit, the import order is
        java
        javax
        3rdparties
        cassandra

        Show
        dbrosius Dave Brosius added a comment - Couple of points of note: This patch doesn't interact with the native interface, which going forward in trunk would seem to be more relevant. At the least that probably means the JMX name shouldn't include 'thrift'. connections? something else. Rather than returning error strings, probably would be better to just throw RuntimeExceptions, as programmatically you wouldn't be able to tell that "No active connection found for given host and port" wasn't a user name. Sure it looks fine in jconsole, but i'm talking about a programatic access. Creating a ThriftSessionManager field in CassandraServer doesn't seem necessary just to make sure that it is loaded first. One would think you could just do something like ThriftSessionManager.instance.hashCode() in the CassandraServer ctor. When i saw the word 'Active' in the jmx names, i immediately assumed you were talking about how many clients had code running for them in the server at the present time. I'm not sure if there's a better word for what you are reporting or not. In addition, with connection pooling, these numbers maybe larger than expected, so is that statistic of use? Or do you really want active use connections? Is there any concern about exposing user identity thru jmx? probably not, just asking. As soon as you do expose this however, admins will want the ability to kill a specific users connection small nit, the import order is java javax 3rdparties cassandra
        Hide
        jbellis Jonathan Ellis added a comment -

        Can you review, Dave?

        Show
        jbellis Jonathan Ellis added a comment - Can you review, Dave?
        Hide
        sureshsajja Suresh added a comment -

        Here is my first take on Cassandra.

        I exposed following attributes and operations via ThriftSessionManagerMBean

        getNoOfActiveSessions(), getActiveConnections()
        getAuthenticatedUserForSession(String host, int port), getKeySpaceForSession(String host, int port)

        Please review the patched attached here.

        Show
        sureshsajja Suresh added a comment - Here is my first take on Cassandra. I exposed following attributes and operations via ThriftSessionManagerMBean getNoOfActiveSessions(), getActiveConnections() getAuthenticatedUserForSession(String host, int port), getKeySpaceForSession(String host, int port) Please review the patched attached here.

          People

          • Assignee:
            mishail Mikhail Stepura
            Reporter:
            rcoli Robert Coli
            Reviewer:
            Jonathan Ellis
          • Votes:
            8 Vote for this issue
            Watchers:
            16 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development