StorageService: fix effectiveOwnership() error reporting #240
Conversation
lersek
commented
Jun 25, 2024
lersek
commented
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 b53be3a ("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 <[email protected]>
Fixes: b53be3a
Signed-off-by: Laszlo Ersek <[email protected]>
|
We are deprecating scylla-jmx, what's the issue this PR is fixing and why fix it? |
|
@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. |
|
To me it was a useful exercise / case study nonetheless! |
|
see also scylladb/scylla-tools-java#398 |
You are right - we did not have a deprecation notice in this repo - which is why I've opened #241 |
|
Obsoleted by the C++ rewrite of |
