scylladb / scylla-jmx

Scylla JMX proxy
GNU Affero General Public License v3.0
29 stars 53 forks source link

StorageService: fix effectiveOwnership() error reporting #240

Closed lersek closed 3 months ago

lersek commented 3 months ago
StorageService: fix effectiveOwnership() error reporting

The originally intended behavior of effectiveOwnership(), for when the
requested keyspace cannot be looked up, can be seen in "scylla-tools-java"
commit d002c7edc58b ("Don't output nonsense ownership without a keyspace",
2014-09-13).

That commit extended the NodeTool client and the StorageService managed
bean at the same time, as follows:

- On the server side, effectiveOwnership() would throw a -- previously not
  thrown -- IllegalArgumentException, when the keyspace was not found.

- On the client side, the IllegalArgumentException would be caught, and
  the client would summarily exit.

The server-side logic regressed when the MBean was rebased to scylladb,
namely in "scylla-jmx" commit b53be3a4ec62 ("StorageService: Add the
effectiveOwnership and getOwnership implementation", 2015-08-24):

(1) A distinct IllegalArgumentException would no longer be thrown (as
    opposed to other IllegalStateExceptions) for "keyspace not found".

(2) Whatever error message getMapInetAddressFloatValue() would emit --
    such as an error message forwarded from scylladb --,
    effectiveOwnership() would suppress that message with the blanket
    string

> Non-system keyspaces don't have the same replication settings, effective
> ownership information is meaningless

    (As a side comment, note that, per the original "scylla-tools-java"
    commit d002c7edc58b above, this constant error string didn't even
    apply when a specific keyspace was requested; it only applied when a
    particular keyspace was *not* requested.)

We cannot easily fix issue (1), because the IllegalStateException-style
representation of any scylladb-side error is determined quite deeply:

  effectiveOwnership()                     [a]
    getMapInetAddressFloatValue()          [b]
      getReader()                          [b]
        getRawValue()                      [b]
          getException()                   [b]
            produces IllegalStateException

[a] src/main/java/org/apache/cassandra/service/StorageService.java
[b] scylla-apiclient/src/main/java/com/scylladb/jmx/api/APIClient.java

Fix issue (2):

- simply allow the IllegalStateException propagate out of the call tree
  cited above, without suppressing its message,

- while at it, remove the exception specification from the
  effectiveOwnership() method, as IllegalStateException need not be a
  checked exception.

As a consequence, the "nodetool status foobar" output improves:

> Note: Scylla API server HTTP GET to URL
> '/storage_service/ownership/foobar' failed: Can't find a keyspace foobar

While just "nodetool status" -- no specific keyspace requested --
preserves the same information:

> Note: Scylla API server HTTP GET to URL
> '/storage_service/ownership/null' failed: std::runtime_error (Non-system
> keyspaces don't have the same replication settings, effective ownership
> information is meaningless)

This latter case will be further improved in a "nodetool" patch (i.e., on
the client side) -- note the nonsensical "null" component in the URL!

Cc: Amnon Heiman <amnon@scylladb.com>
Fixes: b53be3a4ec623dfee51887c4fcd7f8a4d29efeeb
Signed-off-by: Laszlo Ersek <laszlo.ersek@scylladb.com>
mykaul commented 3 months ago

We are deprecating scylla-jmx, what's the issue this PR is fixing and why fix it?

lersek commented 3 months ago

@mykaul the issue is described in the commit message and the PR blurb. And the reason I'm fixing it is ... "by mistake" ;) I didn't know that we were deprecating the java-language tooling; the S101 course in scylladb university still uses them, and I noticed an error in their behavior. If we've stopped fixing bugs in the java tools, then feel free to reject this PR.

lersek commented 3 months ago

To me it was a useful exercise / case study nonetheless!

lersek commented 3 months ago

see also https://github.com/scylladb/scylla-tools-java/pull/398

mykaul commented 3 months ago

@mykaul the issue is described in the commit message and the PR blurb. And the reason I'm fixing it is ... "by mistake" ;) I didn't know that we were deprecating the java-language tooling; the S101 course in scylladb university still uses them, and I noticed an error in their behavior. If we've stopped fixing bugs in the java tools, then feel free to reject this PR.

You are right - we did not have a deprecation notice in this repo - which is why I've opened https://github.com/scylladb/scylla-jmx/issues/241 As for the ScyllaDB university - @guy9 - probably should update S101.

lersek commented 3 months ago

Obsoleted by the C++ rewrite of nodetool; closing.