search

scylladb / scylla-tools-java

Apache Cassandra, supplying tools for Scylla
Apache License 2.0
53 stars 85 forks source link

NodeProbe: allow addressing table name with colon in it #355

Closed nyh closed 12 months ago

nyh commented 1 year ago

As the ObjectName documentation explains, when an addressing an MBean in JMX, when a value contains a comma, equals, colon or quote - it must be quoted, otherwise the ObjectName cannot be constructed.

CQL or Alternator table names cannot contain any of these characters, but in Alternator the names of secondary indexes do contain a colon, so before this patch nodetool cannot address them - it tries to construct an ObjectName with that name, and fails.

Note that "nodetool tablestats" is also broken by this bug, because even if the user doesn't try to specify one specific table, nodetool iterates on all tables and fails when it reaches the view with the colon in its name.

So this patch adds the missing quoting if the table name contains a colon. We don't quote table names that don't have a colon to preserve backward compatibility with old implmentation of JMX that don't know how to unquote these names. A separate patch to scylla-jmx added support for unquoting - see https://github.com/scylladb/scylla-jmx/pull/227.

After this patch "nodetool tablestats" can work even if there is an Alternator GSI or LSI.

nyh commented 1 year ago

See also pull requests in other repositories:

  1. https://github.com/scylladb/scylla-jmx/pull/227 - patch for scylla-jmx to support the quoted table names that this patch can send for a table with a colon in its name.
  2. https://github.com/scylladb/scylla-dtest/pull/3738 - two dtests that reproduce the original bug, and check that it's fixed. This PR makes the nodetool tablestats test work after the JMX PR already made the necessary preperations and made nodetool info work.
denesb commented 12 months ago

@nyh should I wait for https://github.com/scylladb/scylla-jmx/pull/227 to promote in scylla.git, or can I merge right away?

mykaul commented 12 months ago

@nyh should I wait for scylladb/scylla-jmx#227 to promote in scylla.git, or can I merge right away?

It's merged, I think we can merge this one as well.

denesb commented 12 months ago

@nyh should I wait for scylladb/scylla-jmx#227 to promote in scylla.git, or can I merge right away?

It's merged, I think we can merge this one as well.

It is only queued, and it will probably need dequeuing because it is apparently causing problems.

denesb commented 12 months ago

Submodule update PR: https://github.com/scylladb/scylladb/pull/16146