Cassandra
  1. Cassandra
  2. CASSANDRA-3671

provide JMX counters for unavailables/timeouts for reads and writes

    Details

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

      Description

      Attaching patch against trunk.

      1. ASF.LICENSE.NOT.GRANTED--v1-0001-CASSANDRA-3671-trunk-coda-metrics-v2.txt.txt
        96 kB
        Eric Evans
      2. CASSANDRA-3671-trunk-coda-metrics-203-withjar.txt
        94 kB
        Peter Schuller
      3. CASSANDRA-3671-trunk-coda-metrics-v2.txt
        5 kB
        Peter Schuller
      4. CASSANDRA-3671-trunk-coda-metrics-v1.txt
        93 kB
        Peter Schuller
      5. CASSANDRA-3671-trunk-v2.txt
        4 kB
        Peter Schuller
      6. CASSANDRA-3671-trunk.txt
        4 kB
        Peter Schuller

        Issue Links

          Activity

          Hide
          Peter Schuller added a comment -

          Accidentally attached old version of patch. v2 attached which doesn't fail to re-throw in one case.

          Show
          Peter Schuller added a comment - Accidentally attached old version of patch. v2 attached which doesn't fail to re-throw in one case.
          Hide
          Jonathan Ellis added a comment -

          Would this be a good place to start using the Coda Metrics library?

          Show
          Jonathan Ellis added a comment - Would this be a good place to start using the Coda Metrics library?
          Hide
          Peter Schuller added a comment -

          Yes, probably.

          So here's the deal: Other than running into CASSANDRA-3797, there is the problem that we cannot have both the Coda Metrics and our own MBean:s publish under the same class name (StorageProxy in this case).

          I considered various options, and decided to at least submit for consideration something that might be controversial: I have created an org.apache.cassandra.metrics package, containing a ClientRequestMetrics class which has these metrics. The idea is that if we are going to need to adjust naming conventions anyway, let's move to a naming convention which makes sense to the user/system administrator, rather than the implementor ("StorageProxy" means very little to most people who don't know the code, I would suspect).

          It does create a split-world syndrome of "new style" vs. "old style" metrics though. I'd love input.

          Attaching a patch (.jar binary is there on purpose).

          Show
          Peter Schuller added a comment - Yes, probably. So here's the deal: Other than running into CASSANDRA-3797 , there is the problem that we cannot have both the Coda Metrics and our own MBean:s publish under the same class name (StorageProxy in this case). I considered various options, and decided to at least submit for consideration something that might be controversial: I have created an org.apache.cassandra.metrics package, containing a ClientRequestMetrics class which has these metrics. The idea is that if we are going to need to adjust naming conventions anyway, let's move to a naming convention which makes sense to the user/system administrator, rather than the implementor ("StorageProxy" means very little to most people who don't know the code, I would suspect). It does create a split-world syndrome of "new style" vs. "old style" metrics though. I'd love input. Attaching a patch (.jar binary is there on purpose).
          Hide
          Peter Schuller added a comment -

          (Marking "not intended for inclusion because I don't want to create legal hassles due to the inclusion of the .jar file. I will resubmit when it's time to commit if needed.)

          Show
          Peter Schuller added a comment - (Marking "not intended for inclusion because I don't want to create legal hassles due to the inclusion of the .jar file. I will resubmit when it's time to commit if needed.)
          Hide
          Peter Schuller added a comment -

          We could also convert everything in StorageProxy to metrics and leave naming unaltered, assuming metrics can express everything we currently have in the MBean, which I'm skeptical of. Haven't checked, but other than data types like arrays, we provide methods for invocation which is not a metric at all.

          Show
          Peter Schuller added a comment - We could also convert everything in StorageProxy to metrics and leave naming unaltered, assuming metrics can express everything we currently have in the MBean, which I'm skeptical of. Haven't checked, but other than data types like arrays, we provide methods for invocation which is not a metric at all.
          Hide
          Brandon Williams added a comment -

          Can you rebase?

          Show
          Brandon Williams added a comment - Can you rebase?
          Hide
          Peter Schuller added a comment -

          v2 rebased against current trunk.

          Show
          Peter Schuller added a comment - v2 rebased against current trunk.
          Hide
          Brandon Williams added a comment -

          It does create a split-world syndrome of "new style" vs. "old style" metrics though. I'd love input.

          I'm +1 on this approach, because we need to keep things backwards-compatible for existing monitoring solutions, but ultimately I'm unhappy with the current state of JMX and starting fresh like you have here is a good way to solve it. Fleshing out o.a.c.metrics looks like the future to me.

          Show
          Brandon Williams added a comment - It does create a split-world syndrome of "new style" vs. "old style" metrics though. I'd love input. I'm +1 on this approach, because we need to keep things backwards-compatible for existing monitoring solutions, but ultimately I'm unhappy with the current state of JMX and starting fresh like you have here is a good way to solve it. Fleshing out o.a.c.metrics looks like the future to me.
          Hide
          Peter Schuller added a comment -

          Ok. I'll update to the latest release of Codahale and commit once CASSANDRA-3797 is taken care of.

          Show
          Peter Schuller added a comment - Ok. I'll update to the latest release of Codahale and commit once CASSANDRA-3797 is taken care of.
          Hide
          Peter Schuller added a comment -

          Attaching full patch with 2.0.3 and including the .jar (produced with --binary).

          I've run ant artifacts and made sure the resulting .pom file includes the dependency on metrics-core. I am not very familiar with the build+dist process though so would appreciate a +1 that this part seems sufficient.

          For the record, here's how to download the dependency with maven:

          mvn dependency:get -DartifactId=metrics-core -DgroupId=com.yammer.metrics -Dversion=2.0.3 -Dpackaging=jar -DrepoUrl=
          
          Show
          Peter Schuller added a comment - Attaching full patch with 2.0.3 and including the .jar (produced with --binary). I've run ant artifacts and made sure the resulting .pom file includes the dependency on metrics-core. I am not very familiar with the build+dist process though so would appreciate a +1 that this part seems sufficient. For the record, here's how to download the dependency with maven: mvn dependency:get -DartifactId=metrics-core -DgroupId=com.yammer.metrics -Dversion=2.0.3 -Dpackaging=jar -DrepoUrl=
          Hide
          Brandon Williams added a comment -

          +1

          Show
          Brandon Williams added a comment - +1
          Hide
          Eric Evans added a comment - - edited

          I had some problems with the most recent patch (CASSANDRA-3671-trunk-coda-metrics-203-withjar.txt), it seems to be missing src/java/org/apache/cassandra/metrics/ClientRequestMetrics.java.

          v1-0001-CASSANDRA-3671-trunk-coda-metrics-v2.txt.txt is CASSANDRA-3671-trunk-coda-metrics-v2.txt with the jar file added.

          Otherwise, +1 from me.

          Edit: It's not missing the jar, my bad. It is missing the class though.

          Show
          Eric Evans added a comment - - edited I had some problems with the most recent patch ( CASSANDRA-3671 -trunk-coda-metrics-203-withjar.txt), it seems to be missing src/java/org/apache/cassandra/metrics/ClientRequestMetrics.java . v1-0001- CASSANDRA-3671 -trunk-coda-metrics-v2.txt.txt is CASSANDRA-3671 -trunk-coda-metrics-v2.txt with the jar file added. Otherwise, +1 from me. Edit: It's not missing the jar, my bad. It is missing the class though.
          Hide
          Eric Evans added a comment -

          Also, is there any reason this couldn't be applied to the 1.1 branch?

          Show
          Eric Evans added a comment - Also, is there any reason this couldn't be applied to the 1.1 branch?
          Hide
          Brandon Williams added a comment -

          I don't see any reason this can't go in 1.1

          Show
          Brandon Williams added a comment - I don't see any reason this can't go in 1.1
          Hide
          Peter Schuller added a comment -

          Since two people want it in 1.1, I'll go that route (I have nothing against it myself, I was just being conservative).

          Show
          Peter Schuller added a comment - Since two people want it in 1.1, I'll go that route (I have nothing against it myself, I was just being conservative).
          Hide
          Peter Schuller added a comment -

          Committed to 1.1.0, 1.1 and trunk.

          Show
          Peter Schuller added a comment - Committed to 1.1.0, 1.1 and trunk.
          Hide
          Peter Schuller added a comment -

          Why the fix version change, given that it was committed to 1.1.0?

          Show
          Peter Schuller added a comment - Why the fix version change, given that it was committed to 1.1.0?
          Hide
          Brandon Williams added a comment -

          Oops, I thought it wasn't. But really it should not have been since 1.1.0 is frozen.

          Show
          Brandon Williams added a comment - Oops, I thought it wasn't. But really it should not have been since 1.1.0 is frozen.

            People

            • Assignee:
              Peter Schuller
              Reporter:
              Peter Schuller
              Reviewer:
              Brandon Williams
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development