scylla-nodetool: add tablet support for ring command #18608
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
tests/dtest
status/release blocker
denesb
force-pushed
the
scylla-nodetool-ring-tablets
branch
from
04b652a
to
6d159bf
Compare
|
Update:
|
tchaikov
approved these changes
May 10, 2024
🟢 CI State: SUCCESS✅ - Build Build Details:
|
raphaelsc
reviewed
May 10, 2024
raphaelsc
reviewed
May 10, 2024
The primary replica is an arbitrary replica of the tablet's, which is considered to tbe the "main" owner of the tablet, similar to how replicas own tokens in the vnode world. To avoid aliasing the primary replicas with a certain DC or rack, primary replicas are rotated among the tablet's replicas, selecting tablet_id % replica_count as the primary replica.
The tablet variant of the existing get_token_to_endpoint_map(), which returns a list of tablet tokens and the primary replica for each.
…ndpoint Add a keyspace and cf parameter. When specified, the endpoint will return token -> primary replica mapping for the table's tablet tokens, not the vnodes.
Add a table parameter. Pass both keyspace and table (when provided) to the /storage_service/tokens_endpoint API endpoint, so that the returned (and printed) token ring is that of the table's tablets, not the vnode ring. Also pass the table param to the ownership API, which will complain if this param is missing for a tablet keyspace.
Currently, the token count column is always based on the vnodes, which makes no sense for tablet keyspaces. If a tablet keyspace is provided as the keyspace argument, don't print the vnode token count. If the user provided a table argument as well, print the tablet count, otherwise print "?".
After the recent fixes 4 tests started failing with the java nodetool implementation. We are about to ditch the java implementation, but until we actually do, it is valuable to keep the tests passing with both the native and java implementation. So in this patch, these tests are fixed to pass with the java implementation too. There is one test, test_help.py, which fails only if run together with all the tests. I couldn't confirm this 100%, but it seems like this is due to JMX sending a rouge request on some timer, which happens to hit this test. I don't think this is worth trying to fix.
|
Update:
|
denesb
force-pushed
the
scylla-nodetool-ring-tablets
branch
from
6d159bf
to
a77796f
Compare
🟢 CI State: SUCCESS✅ - Build Build Details:
|
|
@scylladb/scylla-maint ping. |
raphaelsc
approved these changes
May 15, 2024
avikivity
reviewed
May 15, 2024
| @@ -357,6 +357,9 @@ public: | |||
| /// \throws std::logic_error If the given id does not belong to this instance. | |||
| dht::token_range get_token_range(tablet_id id) const; | |||
|
|
|||
| /// Returns the primary replica for the tablet | |||
| host_id get_primary_replica(tablet_id id) const; | |||
|
|
|||
There was a problem hiding this comment.
@asias @bhalevy we should use this for repair. Does it need to be integrated into effective_replication_map?
you mean to have such a function for vnodes?
i don't think it's necesaary since tablet repair has it's own path that can diverge from vnodes.
Also, for repair, we also need a variant of this function (or an optional parameter to it, to select a primary replica within a given dc)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, invoking
nodetool ringon a tablet keyspace fails with an error, because it doesn't pass the required table parameter to/storage_service/ownership/{keyspace}. Further to this, the command will currently always output the vnode ring, regardless of the keyspace and table parameter. This series fixes this, adding tablet support to/storage_service/tokens_endpoint, which will now return the tablet ring (tablet token -> tablet primary replica mapping) if the new keyspace and table parameters are provided.nodetool statusalso gets a touch-up, to provide the tablet ring's token count (the tablet count) when invoked with a tablet keyspace and table.Fixes: #17889
Fixes: #18474