diff --git a/cql3/statements/alter_keyspace_statement.cc b/cql3/statements/alter_keyspace_statement.cc index 1e7636a8cb..429b5ea21c 100644 --- a/cql3/statements/alter_keyspace_statement.cc +++ b/cql3/statements/alter_keyspace_statement.cc @@ -44,16 +44,15 @@ future<> cql3::statements::alter_keyspace_statement::check_access(query_processo return state.has_keyspace_access(_name, auth::permission::ALTER); } -// We want to ensure that each DC's RF is going to change by at most 1 -static bool validate_rf_difference(const std::string& curr_rf, const std::string& new_rf) { +static unsigned get_abs_rf_diff(const std::string& curr_rf, const std::string& new_rf) { try { - return std::abs(std::stoi(curr_rf) - std::stoi(new_rf)) <= 1; + return std::abs(std::stoi(curr_rf) - std::stoi(new_rf)); } catch (std::invalid_argument const& ex) { - on_internal_error(mylogger, format("validate_rf_difference expects integer arguments, " - "but got curr_rf:{} and new_rf:{}", curr_rf, new_rf)); + on_internal_error(mylogger, fmt::format("get_abs_rf_diff expects integer arguments, " + "but got curr_rf:{} and new_rf:{}", curr_rf, new_rf)); } catch (std::out_of_range const& ex) { - on_internal_error(mylogger, format("validate_rf_difference expects integer arguments to fit into `int` type, " - "but got curr_rf:{} and new_rf:{}", curr_rf, new_rf)); + on_internal_error(mylogger, fmt::format("get_abs_rf_diff expects integer arguments to fit into `int` type, " + "but got curr_rf:{} and new_rf:{}", curr_rf, new_rf)); } } @@ -87,6 +86,7 @@ void cql3::statements::alter_keyspace_statement::validate(query_processor& qp, c const std::map& current_rf_per_dc = ks.metadata()->strategy_options(); auto new_rf_per_dc = _attrs->get_replication_options(); new_rf_per_dc.erase(ks_prop_defs::REPLICATION_STRATEGY_CLASS_KEY); + unsigned total_abs_rfs_diff = 0; for (const auto& [new_dc, new_rf] : new_rf_per_dc) { sstring old_rf = "0"; if (auto new_dc_in_current_mapping = current_rf_per_dc.find(new_dc); @@ -99,8 +99,8 @@ void cql3::statements::alter_keyspace_statement::validate(query_processor& qp, c // first we need to report non-existing DCs, then if RFs aren't changed by too much. continue; } - if (!validate_rf_difference(old_rf, new_rf)) { - throw exceptions::invalid_request_exception("Cannot modify replication factor of any DC by more than 1 at a time."); + if (total_abs_rfs_diff += get_abs_rf_diff(old_rf, new_rf); total_abs_rfs_diff >= 2) { + throw exceptions::invalid_request_exception("Only one DC's RF can be changed at a time and not by more than 1"); } } } diff --git a/test/topology_custom/test_tablets.py b/test/topology_custom/test_tablets.py index 4c216217b7..594d6f6868 100644 --- a/test/topology_custom/test_tablets.py +++ b/test/topology_custom/test_tablets.py @@ -199,26 +199,44 @@ async def test_tablet_mutation_fragments_unowned_partition(manager: ManagerClien async def test_multidc_alter_tablets_rf(request: pytest.FixtureRequest, manager: ManagerClient) -> None: config = {"endpoint_snitch": "GossipingPropertyFileSnitch", "enable_tablets": "true"} - logger.info("Creating a new cluster of 1 node in 1st DC and 2 nodes in 2nd DC") - await manager.servers_add(3, config=config, property_file={'dc': f'dc1', 'rack': 'myrack'}) + logger.info("Creating a new cluster of 2 nodes in 1st DC and 2 nodes in 2nd DC") + # we have to have at least 2 nodes in each DC if we want to try setting RF to 2 in each DC + await manager.servers_add(2, config=config, property_file={'dc': f'dc1', 'rack': 'myrack'}) await manager.servers_add(2, config=config, property_file={'dc': f'dc2', 'rack': 'myrack'}) - # await manager.server_add(config=config, property_file={'dc': f'dc2', 'rack': 'myrack'}) cql = manager.get_cql() await cql.run_async("create keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 1}") # need to create a table to not change only the schema, but also tablets replicas await cql.run_async("create table ks.t (pk int primary key)") - with pytest.raises(InvalidRequest, match="Cannot modify replication factor of any DC by more than 1 at a time."): + with pytest.raises(InvalidRequest, match="Only one DC's RF can be changed at a time and not by more than 1"): # changing RF of dc2 from 0 to 2 should fail await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc2': 2}") # changing RF of dc2 from 0 to 1 should succeed await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc2': 1}") - # we want to ensure that RFs of both DCs are equal to 1 now + # ensure that RFs of both DCs are equal to 1 now, i.e. that omitting dc1 in above command didn't change it res = await cql.run_async("SELECT * FROM system_schema.keyspaces WHERE keyspace_name = 'ks'") assert res[0].replication['dc1'] == '1' assert res[0].replication['dc2'] == '1' + # incrementing RF of 2 DCs at once should NOT succeed, because it'd leave 2 pending tablets replicas + with pytest.raises(InvalidRequest, match="Only one DC's RF can be changed at a time and not by more than 1"): + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 2, 'dc2': 2}") + # as above, but decrementing + with pytest.raises(InvalidRequest, match="Only one DC's RF can be changed at a time and not by more than 1"): + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 0, 'dc2': 0}") + # as above, but decrement 1 RF and increment the other + with pytest.raises(InvalidRequest, match="Only one DC's RF can be changed at a time and not by more than 1"): + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 2, 'dc2': 0}") + # as above, but RFs are swapped + with pytest.raises(InvalidRequest, match="Only one DC's RF can be changed at a time and not by more than 1"): + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 0, 'dc2': 2}") + + # check that we can remove all replicas from dc2 by changing RF from 1 to 0 + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc2': 0}") + # check that we can remove all replicas from the cluster, i.e. change RF of dc1 from 1 to 0 as well: + await cql.run_async("alter keyspace ks with replication = {'class': 'NetworkTopologyStrategy', 'dc1': 0}") + # Reproducer for https://github.com/scylladb/scylladb/issues/18110 # Check that an existing cached read, will be cleaned up when the tablet it reads