Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

raft topology: don't register topology-on-raft RPCs in non-topology-on-raft mode #15862

Closed
Tracked by #15555
piodul opened this issue Oct 27, 2023 · 2 comments · Fixed by #15917
Closed
Tracked by #15555

raft topology: don't register topology-on-raft RPCs in non-topology-on-raft mode #15862

piodul opened this issue Oct 27, 2023 · 2 comments · Fixed by #15917
Assignees
@kostja @piodul @kbr-scylla
Labels
area/raft area/topology changes

Comments

@piodul
Copy link
Contributor

piodul commented Oct 27, 2023

Sub-task of #15555, followup to #15196 (comment).

If topology on raft mode is disabled, there is no use in registering RPCs that are exclusive to that mode. We should adjust the code in storage_service not to do that and make sure that the existing code does not accidentally try to send those RPCs when raft mode is disabled.
@piodul piodul mentioned this issue Oct 27, 2023
Closed
@kbr-scylla
Copy link
Contributor

make sure that the existing code does not accidentally try to send those RPCs when raft mode is disabled.

In fact, that would be dangerous if we release a version that sends those RPCs out. We then wouldn't be able to modify these RPCs in backward-incompatible way before going out of experimental mode!

We should be quick here and backport to 5.4 if necessary

@mykaul mykaul added Backport candidate backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels Oct 28, 2023
@patjed41 patjed41 removed their assignment Oct 31, 2023
@piodul piodul mentioned this issue Nov 2, 2023
Merged
kbr-scylla added a commit that referenced this issue Nov 3, 2023
@kbr-scylla
Merge 'raft: topology: outside topology-on-raft mode, make sure not t…
9c37f5e 
…o use its RPCs' from Piotr Dulikowski

Topology on raft is still an experimental feature. The RPC verbs
introduced in that mode shouldn't be used when it's disabled, otherwise
we lose the right to make breaking changes to those verbs.

First, make sure that the aforementioned verbs are not sent outside the
mode. It turns out that `raft_pull_topology_snapshot` could be sent
outside topology-on-raft mode - after the PR, it no longer can.

Second, topology-on-raft mode verbs are now not registered at all on the
receiving side when the mode is disabled.

Additionally tested by running `topology/` tests with
`consistent_cluster_management: True` but with experimental features
disabled.

Fixes: #15862

Closes #15917

* github.com:scylladb/scylladb:
  storage_service: fix indentation
  raft: topology: only register verbs in topology-on-raft mode
  raft: topology: only pull topology snapshot in topology-on-raft mode

(cherry picked from commit 5cf18b1)
@kbr-scylla
Copy link
Contributor

Queued backport to 5.4

@mykaul mykaul removed Backport candidate backport/5.4 Issues that should be backported to 5.4 branch once they'll be fixed labels Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kostja kostja

@piodul piodul

@kbr-scylla kbr-scylla

Labels
area/raft area/topology changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

raft: topology: outside topology-on-raft mode, make sure not to use its RPCs piodul/scylla
5 participants
@kostja @mykaul @piodul @patjed41 @kbr-scylla