Some nodetool commands break when there is an Alternator GSI in the database

[nyh]

We already have an issue about this in a different repository, scylladb/scylla-jmx#226 because the fix is in scylla-jmx and scylla-tools-java, not scylladb core. But I'm nevertheless opening an issue also here because we will need to update the scylla-jmx and scylla-tools-java submodules - and this update will "Fixes" this issue. This will allow us to also track the backporting of this update.

Normally, CQL table names may only have alphanumeric characters or underscores. The DynamoDB API also adds dash and dot as allowed characters. But when Alternator creates a GSI or LSI, the materialized view name will also contain a ":" or "!" characters, respectively.

It turns out that JMX, used by nodetool, saves the Java Bean "address" of table names as Java ObjectName, which is constructed with a string like org.apache.cassandra.db:type=tname,keyspace=ksname,columnfamily=cfname. But when the cfname string contains a colon, it must be quoted. As the ObjectName documentation explains:

Each value associated with a key is a string of characters that is either unquoted or quoted.
An unquoted value is a possibly empty string of characters which may not contain any of the characters comma, equals, colon, or quote.

Of these characters (comma, equals, colon or quote), only the colon is actually possible in table names.

If we don't quote these names, two kinds of bugs can happen:

1. Nodetool may fail when it tries to create an ObjectName to send to the JMX server.
2. The JMX server may fail when it internally tries to list tables and save them as ObjectName.

There are three PRs that fix the known bugs:

• Fix handling of table names with colon characters scylla-jmx#227
• ColumnFamilyStore: only quote table names if necessary scylla-jmx#230
• NodeProbe: allow addressing table name with colon in it scylla-tools-java#355

And also a dtest PR that reproduce the bug - and verify the fix - specifically in "nodetool info" and "nodetool tablestates". We may need to write additional tests later (and it's possible we didn't fix all the nodetool bugs in this area...).

• https://github.com/scylladb/scylla-dtest/pull/3738

[eliransin]

@scylladb/scylla-maint can someone handle the backport?
/cc @mykaul

[denesb]

@scylladb/scylla-maint can someone handle the backport? /cc @mykaul

The author is a maintainer himself, so he is the best candidate to do the backport.

[nyh]

@scylladb/scylla-maint can someone handle the backport? /cc @mykaul

The author is a maintainer himself, so he is the best candidate to do the backport.

Backporting with all these submodules is no fun, but I accept the challenge. I think because of all these submodules I can't really run CI before merging the various backport, so I hope I don't mess this up.

[denesb]

@scylladb/scylla-maint can someone handle the backport? /cc @mykaul

The author is a maintainer himself, so he is the best candidate to do the backport.

Backporting with all these submodules is no fun, but I accept the challenge. I think because of all these submodules I can't really run CI before merging the various backport, so I hope I don't mess this up.

If you are not sure, you can create a PR in scylla.git for the submodule update, and merge it yourself after CI passes.
I did the same when I backported the dependency updates.

[nyh]

@denesb you're right, but it still requires that I merge everything into the submodules first, without a PR. I'll think what is the least ugly and/or risky way to do this.

[mykaul]

@denesb you're right, but it still requires that I merge everything into the submodules first, without a PR. I'll think what is the least ugly and/or risky way to do this.

Isn't the core issue that submodules do not have a meaningful CI?

[denesb]

@denesb you're right, but it still requires that I merge everything into the submodules first, without a PR. I'll think what is the least ugly and/or risky way to do this.

Yes, you have to cherry-pick the fixes onto the branch-X of the submodule, then push it to origin, and only after that create the PR in sscylla.git. You are right that if CI fails, you already pushed the branches in the submodule. Note that since the submodules are updated with explicit commits, we have a bit more freedom with force-pushes and we can even waive the bisectability requirement for submodule branches. If you find something went wrong, you can backport additional commits, or edit and force-push the existing commit.

[denesb]

@denesb you're right, but it still requires that I merge everything into the submodules first, without a PR. I'll think what is the least ugly and/or risky way to do this.

Isn't the core issue that submodules do not have a meaningful CI?

Yes, that is one of the issues. We merge/backport directly to master/branch-X not to next[-X].

[nyh]

Merged into master submodules.
PR for the submodule updates:
#16294 (5.4)
#16296 (5.2)

When CI passes on those, I can merge them...

[nyh]

@denesb you're right, but it still requires that I merge everything into the submodules first, without a PR. I'll think what is the least ugly and/or risky way to do this.

Yes, you have to cherry-pick the fixes onto the branch-X of the submodule, then push it to origin, and only after that create the PR in sscylla.git. You are right that if CI fails, you already pushed the branches in the submodule. Note that since the submodules are updated with explicit commits, we have a bit more freedom with force-pushes and we can even waive the bisectability requirement for submodule branches. If you find something went wrong, you can backport additional commits, or edit and force-push the existing commit.

Right, I know all this, but it's still extremely ugly and error-prone. Even if an error doesn't mean absolute disaster, it's still annoying, and is especially problematic if multiple people are trying to backport in parallel, which I admit isn't common. Anyway, I'll manage :-)

[nyh]

Merged into master submodules. PR for the submodule updates: #16294 (5.4) #16296 (5.2)

When CI passes on those, I can merge them...

The two CI runs passed, and I now merged the tools/java and tools/jmx changes into the scylladb.git next-5.2 and next-5.4 branches. They should hopefully be promoted to branch-5.2 and branch-5.4 soon (which should be successful because the CI already tested that I didn't goof up the backport).

Please note that I did NOT backport the dtest commit (see https://github.com/scylladb/scylla-dtest/pull/3738) that verifies that these patches actually fixes the original bug.

[nyh]

@kostja what is the meaning of assigning this closed issue to me? As I noted above, I already backported it to 5.2 and 5.4.
What else would you want me to do on this issue?
Did you perhaps intend to assign me some non-open-source-branch issue, not this one?

[kostja]

You worked on this issue, so I assigned it to you. It's good for the stats, and for getting notifications (sometimes there are notifications on closed issues).

[nyh]

Oh :-)