From 8a612da1555eb4ad4bf7a8e704a3b53e2a1084b9 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 27 May 2024 16:20:58 +0300 Subject: [PATCH 1/3] cql3: Remove unused ks_prop_defs/prepare_options() argument Signed-off-by: Pavel Emelyanov --- cql3/statements/ks_prop_defs.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cql3/statements/ks_prop_defs.cc b/cql3/statements/ks_prop_defs.cc index 32ce97117b..141923af35 100644 --- a/cql3/statements/ks_prop_defs.cc +++ b/cql3/statements/ks_prop_defs.cc @@ -24,7 +24,6 @@ static std::map prepare_options( const sstring& strategy_class, const locator::token_metadata& tm, std::map options, - std::optional& initial_tablets, const std::map& old_options = {}) { options.erase(ks_prop_defs::REPLICATION_STRATEGY_CLASS_KEY); @@ -162,7 +161,7 @@ std::optional ks_prop_defs::get_replication_strategy_class() const { lw_shared_ptr ks_prop_defs::as_ks_metadata(sstring ks_name, const locator::token_metadata& tm, const gms::feature_service& feat) { auto sc = get_replication_strategy_class().value(); std::optional initial_tablets = get_initial_tablets(sc, feat.tablets); - auto options = prepare_options(sc, tm, get_replication_options(), initial_tablets); + auto options = prepare_options(sc, tm, get_replication_options()); return data_dictionary::keyspace_metadata::new_keyspace(ks_name, sc, std::move(options), initial_tablets, get_boolean(KW_DURABLE_WRITES, true), get_storage_options()); } @@ -174,7 +173,7 @@ lw_shared_ptr ks_prop_defs::as_ks_metadata_u std::optional initial_tablets; if (sc) { initial_tablets = get_initial_tablets(*sc, old->initial_tablets().has_value()); - options = prepare_options(*sc, tm, get_replication_options(), initial_tablets, old_options); + options = prepare_options(*sc, tm, get_replication_options(), old_options); } else { sc = old->strategy_name(); options = old_options; From a172ef1bdff49299d6f0a6e389695a2a2350aeda Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 27 May 2024 16:25:09 +0300 Subject: [PATCH 2/3] cql3: Fix parsing of ALTER KEYSPACE's tablets parameters When the `WITH` doesn't include the `replication` parameters, the `tablets` one is ignoded, even if it's present in the statement. That's not great, those two parameter sets are pretty much independent and should be parsed individually. Signed-off-by: Pavel Emelyanov --- cql3/statements/ks_prop_defs.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cql3/statements/ks_prop_defs.cc b/cql3/statements/ks_prop_defs.cc index 141923af35..861cd81223 100644 --- a/cql3/statements/ks_prop_defs.cc +++ b/cql3/statements/ks_prop_defs.cc @@ -170,13 +170,14 @@ lw_shared_ptr ks_prop_defs::as_ks_metadata_u std::map options; const auto& old_options = old->strategy_options(); auto sc = get_replication_strategy_class(); - std::optional initial_tablets; if (sc) { - initial_tablets = get_initial_tablets(*sc, old->initial_tablets().has_value()); options = prepare_options(*sc, tm, get_replication_options(), old_options); } else { sc = old->strategy_name(); options = old_options; + } + std::optional initial_tablets = get_initial_tablets(*sc, old->initial_tablets().has_value()); + if (!initial_tablets) { initial_tablets = old->initial_tablets(); } From 1003391ed63e4d4988fedbef7045db3353981939 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 27 May 2024 16:21:52 +0300 Subject: [PATCH 3/3] cql-pytest: Add validation of ALTER KEYSPACE WITH TABLETS There's a test that checks how ALTER changes the initial tablets value, but it equips the statement with `replication` parameters because of limitations that parser used to impose. Now the `tablets` parameters can come on their own, so add a new test. The old one is kept from compatibility considerations. Signed-off-by: Pavel Emelyanov --- test/cql-pytest/test_tablets.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/cql-pytest/test_tablets.py b/test/cql-pytest/test_tablets.py index 5ee80f68e4..96cb23c89a 100644 --- a/test/cql-pytest/test_tablets.py +++ b/test/cql-pytest/test_tablets.py @@ -100,6 +100,20 @@ def test_alter_changes_initial_tablets(cql, skip_without_tablets): assert res.initial_tablets == 2 +def test_alter_changes_initial_tablets_short(cql, skip_without_tablets): + ksdef = "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1} AND tablets = {'initial': 1};" + with new_test_keyspace(cql, ksdef) as keyspace: + orig_rep = cql.execute(f"SELECT replication FROM system_schema.keyspaces WHERE keyspace_name = '{keyspace}'").one() + + cql.execute(f"ALTER KEYSPACE {keyspace} WITH tablets = {{'initial': 2}};") + res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one() + assert res.initial_tablets == 2 + + # Test that replication parameters didn't change + rep = cql.execute(f"SELECT replication FROM system_schema.keyspaces WHERE keyspace_name = '{keyspace}'").one() + assert rep.replication == orig_rep.replication + + # Test that initial number of tablets is preserved in describe def test_describe_initial_tablets(cql, skip_without_tablets): ksdef = "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', 'replication_factor' : '1' } " \