[PATCH v1 02/33] storage_service: drop outdated code that checks id raft topology should be used

[Gleb Natapov]

After raft_topology_change_enabled() was introduced the code does
nothing useful. The function is responsible for teh decision if raft topology
is enabled or not.
---
service/storage_service.cc | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/service/storage_service.cc b/service/storage_service.cc
index f0b9ff5f7d0..c3be166dd6c 100644
--- a/service/storage_service.cc
+++ b/service/storage_service.cc
@@ -1771,20 +1771,7 @@ future<> storage_service::join_topology(sharded<service::storage_proxy>& proxy,
co_await _group0->setup_group0(_sys_ks.local(), initial_contact_nodes, std::move(handshaker),
raft_replace_info, *this, _qp, _migration_manager.local(), raft_topology_change_enabled(), join_params);

- raft::server* raft_server = co_await [this] () -> future<raft::server*> {
- if (!raft_topology_change_enabled()) {
- co_return nullptr;
- } else if (_sys_ks.local().bootstrap_complete()) {
- auto [upgrade_lock_holder, upgrade_state] = co_await _group0->client().get_group0_upgrade_state();
- co_return upgrade_state == group0_upgrade_state::use_post_raft_procedures ? &_group0->group0_server() : nullptr;
- } else {
- auto upgrade_state = (co_await _group0->client().get_group0_upgrade_state()).second;
- if (upgrade_state != group0_upgrade_state::use_post_raft_procedures) {
- on_internal_error(rtlogger, "cluster not upgraded to use group 0 after setup_group0");
- }
- co_return &_group0->group0_server();
- }
- } ();
+ raft::server* raft_server = raft_topology_change_enabled() ? &_group0->group0_server() : nullptr;

if (!raft_topology_change_enabled()) {
co_await _gossiper.wait_for_gossip_to_settle();
--
2.47.1

[Benny Halevy]

On Mon, 2025-02-24 at 13:04 +0200, 'Gleb Natapov' via ScyllaDB development wrote:
> After raft_topology_change_enabled() was introduced the code does
> nothing useful. The function is responsible for teh decision if raft topology

nit s/teh/the/

And in patch title s/id/if/ (I think)

[Gleb Natapov]

On Thu, Feb 27, 2025 at 01:12:27PM +0200, Benny Halevy wrote:
> On Mon, 2025-02-24 at 13:04 +0200, 'Gleb Natapov' via ScyllaDB development wrote:
> > After raft_topology_change_enabled() was introduced the code does
> > nothing useful. The function is responsible for teh decision if raft topology
>
> nit s/teh/the/
>
> And in patch title s/id/if/ (I think)
>

You think right.

--
Gleb.

[Avi Kivity]

On Mon, 2025-02-24 at 13:04 +0200, 'Gleb Natapov' via ScyllaDB
development wrote:

This code is hard to review, because the relationship between
raft_topology_change_enabled() and _group0 is not clear. The reviewer
doesn't know if they are updated atomically, or if one is updated after
the other, which makes the results wrong.

It would be better to have a raft_group_server_if_enabled() function
that does both.

[Gleb Natapov]

_group0 is mandatory by now, so it always exists. Topology changes over
raft can be disabled or enabled. Even before _group0 was mandatory
raft_topology_change_enabled() could have been set to true only if raft is
enabled.

> It would be better to have a raft_group_server_if_enabled() function
> that does both.
>

I am not sure what do you want the function to do.

--
Gleb.

[Avi Kivity]

These kinds of hidden relationships make code review (and refactoring)
difficult.

> > It would be better to have a raft_group_server_if_enabled()
> > function
> > that does both.
> >
> I am not sure what do you want the function to do.
>

Return a disengaged optional (if disabled) and engaged optional with
the wanted reference (if disabled).

[Gleb Natapov]

But we return a pointer here. It is already optional. Do you want me to
hide

raft::server* raft_server = raft_topology_change_enabled() ? &_group0->group0_server() : nullptr;

in raft_group_server_if_enabled() function? I can do that.


--
Gleb.

[Avi Kivity]

Yes, it's clearer than two functions (that the reader can't guess the relationship between them). But no need to do so here.