[QUEUED scylladb next] cql: forbid switching from tablets to vnodes in ALTER KS

[Commit Bot]

From: Piotr Smaron <piotr....@scylladb.com>
Committer: Pavel Emelyanov <xe...@scylladb.com>
Branch: next

cql: forbid switching from tablets to vnodes in ALTER KS

This check is already in place, but isn't fully working, i.e.
switching from a vnode KS to a tablets KS is not allowed, but
this check doesn't work in the other direction. To fix the
latter, `ks_prop_defs::get_initial_tablets()` has been changed
to handle 3 states: (1) init_tablets is set, (2) it was skipped,
(3) tablets are disabled. These couldn't fit into std::optional,
so a new local struct to hold these states has been introduced.
Callers of this function have been adjusted to set init_tablets
to an appropriate value according to the circumstances, i.e. if
tablets are globally enabled, but have been skipped in the CQL,
init_tablets is automatically set to 0, but if someone executes
ALTER KS and doesn't provide tablets options, they're inherited
from the old KS.
I tried various approaches and this one resulted in the least
lines of code changed. I also provided testcases to explain how
the code behaves.

Fixes: #18795

Closes scylladb/scylladb#19368

---
diff --git a/cql3/statements/ks_prop_defs.cc b/cql3/statements/ks_prop_defs.cc
--- a/cql3/statements/ks_prop_defs.cc
+++ b/cql3/statements/ks_prop_defs.cc
@@ -138,28 +138,27 @@ data_dictionary::storage_options ks_prop_defs::get_storage_options() const {
return opts;
}

-std::optional<unsigned> ks_prop_defs::get_initial_tablets(const sstring& strategy_class, bool enabled_by_default) const {
+ks_prop_defs::init_tablets_options ks_prop_defs::get_initial_tablets(const sstring& strategy_class, bool enabled_by_default) const {
// FIXME -- this should be ignored somehow else
+ init_tablets_options ret{ .enabled = false, .specified_count = std::nullopt };
if (locator::abstract_replication_strategy::to_qualified_class_name(strategy_class) != "org.apache.cassandra.locator.NetworkTopologyStrategy") {
- return std::nullopt;
+ return ret;
}

auto tablets_options = get_map(KW_TABLETS);
if (!tablets_options) {
- return enabled_by_default ? std::optional<unsigned>(0) : std::nullopt;
+ return enabled_by_default ? init_tablets_options{ .enabled = true } : ret;
}

- std::optional<unsigned> ret;
-
auto it = tablets_options->find("enabled");
if (it != tablets_options->end()) {
auto enabled = it->second;
tablets_options->erase(it);

if (enabled == "true") {
- ret = 0; // even if 'initial' is not set, it'll start with auto-detection
+ ret = init_tablets_options{ .enabled = true, .specified_count = 0 }; // even if 'initial' is not set, it'll start with auto-detection
} else if (enabled == "false") {
- assert(!ret.has_value());
+ assert(!ret.enabled);
return ret;
} else {
throw exceptions::configuration_exception(sstring("Tablets enabled value must be true or false; found: ") + enabled);
@@ -169,7 +168,7 @@ std::optional<unsigned> ks_prop_defs::get_initial_tablets(const sstring& strateg
it = tablets_options->find("initial");
if (it != tablets_options->end()) {
try {
- ret = std::stol(it->second);
+ ret = init_tablets_options{ .enabled = true, .specified_count = std::stol(it->second)};
} catch (...) {
throw exceptions::configuration_exception(sstring("Initial tablets value should be numeric; found ") + it->second);
}
@@ -209,10 +208,14 @@ std::map<sstring, sstring> ks_prop_defs::get_all_options_flattened(const gms::fe

lw_shared_ptr<data_dictionary::keyspace_metadata> 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<unsigned> initial_tablets = get_initial_tablets(sc, feat.tablets);
+ auto initial_tablets = get_initial_tablets(sc, feat.tablets);
+ // if tablets options have not been specified, but tablets are globally enabled, set the value to 0
+ if (initial_tablets.enabled && !initial_tablets.specified_count) {
+ initial_tablets.specified_count = 0;
+ }
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());
+ std::move(options), initial_tablets.specified_count, get_boolean(KW_DURABLE_WRITES, true), get_storage_options());
}

lw_shared_ptr<data_dictionary::keyspace_metadata> ks_prop_defs::as_ks_metadata_update(lw_shared_ptr<data_dictionary::keyspace_metadata> old, const locator::token_metadata& tm, const gms::feature_service& feat) {
@@ -225,12 +228,13 @@ lw_shared_ptr<data_dictionary::keyspace_metadata> ks_prop_defs::as_ks_metadata_u
sc = old->strategy_name();
options = old_options;
}
- std::optional<unsigned> initial_tablets = get_initial_tablets(*sc, old->initial_tablets().has_value());
- if (!initial_tablets) {
- initial_tablets = old->initial_tablets();
+ auto initial_tablets = get_initial_tablets(*sc, old->initial_tablets().has_value());
+ // if tablets options have not been specified, inherit them if it's tablets-enabled KS
+ if (initial_tablets.enabled && !initial_tablets.specified_count) {
+ initial_tablets.specified_count = old->initial_tablets();
}

- return data_dictionary::keyspace_metadata::new_keyspace(old->name(), *sc, options, initial_tablets, get_boolean(KW_DURABLE_WRITES, true), get_storage_options());
+ return data_dictionary::keyspace_metadata::new_keyspace(old->name(), *sc, options, initial_tablets.specified_count, get_boolean(KW_DURABLE_WRITES, true), get_storage_options());
}


diff --git a/cql3/statements/ks_prop_defs.hh b/cql3/statements/ks_prop_defs.hh
--- a/cql3/statements/ks_prop_defs.hh
+++ b/cql3/statements/ks_prop_defs.hh
@@ -49,13 +49,18 @@ public:
private:
std::optional<sstring> _strategy_class;
public:
+ struct init_tablets_options {
+ bool enabled;
+ std::optional<unsigned> specified_count;
+ };
+
ks_prop_defs() = default;
explicit ks_prop_defs(std::map<sstring, sstring> options);

void validate();
std::map<sstring, sstring> get_replication_options() const;
std::optional<sstring> get_replication_strategy_class() const;
- std::optional<unsigned> get_initial_tablets(const sstring& strategy_class, bool enabled_by_default) const;
+ init_tablets_options get_initial_tablets(const sstring& strategy_class, bool enabled_by_default) const;
data_dictionary::storage_options get_storage_options() const;
bool get_durable_writes() const;
std::map<sstring, sstring> get_all_options_flattened(const gms::feature_service& feat) const;
diff --git a/service/topology_coordinator.cc b/service/topology_coordinator.cc
--- a/service/topology_coordinator.cc
+++ b/service/topology_coordinator.cc
@@ -816,7 +816,7 @@ class topology_coordinator : public endpoint_lifecycle_subscriber {
if (error.empty()) {
const sstring strategy_name = "NetworkTopologyStrategy";
auto ks_md = keyspace_metadata::new_keyspace(ks_name, strategy_name, repl_opts,
- new_ks_props.get_initial_tablets(strategy_name, true),
+ new_ks_props.get_initial_tablets(strategy_name, true).specified_count,
new_ks_props.get_durable_writes(), new_ks_props.get_storage_options());
auto schema_muts = prepare_keyspace_update_announcement(_db, ks_md, guard.write_timestamp());
for (auto& m: schema_muts) {
diff --git a/test/cql-pytest/test_tablets.py b/test/cql-pytest/test_tablets.py
--- a/test/cql-pytest/test_tablets.py
+++ b/test/cql-pytest/test_tablets.py
@@ -56,7 +56,7 @@ def test_alter_cannot_change_vnodes_to_tablets(cql, skip_without_tablets):


# Converting vnodes-based keyspace to tablets-based in not implemented yet
-def test_alter_doesnt_enable_tablets(cql, skip_without_tablets):
+def test_alter_vnodes_ks_doesnt_enable_tablets(cql, skip_without_tablets):
ksdef = "WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};"
with new_test_keyspace(cql, ksdef) as keyspace:
cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy'}};")
@@ -68,6 +68,24 @@ def test_alter_doesnt_enable_tablets(cql, skip_without_tablets):
assert len(list(res)) == 0, "tablets replication strategy turned on"


+# Converting tablets-based keyspace to vnodes-based in not implemented yet
+def test_alter_cannot_change_tablets_to_vnodes(cql, this_dc, skip_without_tablets):
+ ksdef = f"WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND TABLETS = {{ 'enabled' : true }}"
+ with new_test_keyspace(cql, ksdef) as keyspace:
+ with pytest.raises(InvalidRequest, match="Cannot alter replication strategy vnode/tablets flavor"):
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'enabled': false}};")
+
+
+# Converting tablets-based keyspace to vnodes-based in not implemented yet
+def test_alter_tablets_ks_doesnt_disable_tablets(cql, this_dc, skip_without_tablets):
+ ksdef = f"WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND TABLETS = {{ 'enabled' : true }}"
+ with new_test_keyspace(cql, ksdef) as keyspace:
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}};")
+
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'")
+ assert len(list(res)) == 1, "tablets replication strategy turned off"
+
+
def test_tablet_default_initialization(cql, skip_without_tablets):
ksdef = "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1};"
with new_test_keyspace(cql, ksdef) as keyspace:
@@ -95,10 +113,36 @@ def test_tablets_can_be_explicitly_disabled(cql, skip_without_tablets):
def test_alter_changes_initial_tablets(cql, this_dc, skip_without_tablets):
ksdef = f"WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 1}};"
with new_test_keyspace(cql, ksdef) as keyspace:
+ # 1 -> 2, i.e. can change to a different positive integer from some positive integer
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 2}};")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 2
+
+ # 2 -> 0, i.e. can change from a positive int to zero
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 0}};")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 0
+
+ # 0 -> 2, i.e. can change from zero to a positive int
cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 2}};")
res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
assert res.initial_tablets == 2

+ # 2 -> 0, i.e. providing only {'enable': true} zeroes init_tablets
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'enabled': true}};")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 0
+
+ # 0 -> 2, i.e. providing 'enabled' & 'initial' combined sets init_tablets to 'initial'
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'enabled': true, 'initial': 2}};")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 2
+
+ # 2 -> 0, i.e. providing 'enabled' & 'initial' = 0 zeroes init_tablets
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'enabled': true, 'initial': 0}};")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 0
+

def test_alter_changes_initial_tablets_short(cql, skip_without_tablets):
ksdef = "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1} AND tablets = {'initial': 1};"
@@ -114,6 +158,23 @@ def test_alter_changes_initial_tablets_short(cql, skip_without_tablets):
assert rep.replication == orig_rep.replication


+def test_alter_preserves_tablets_if_initial_tablets_skipped(cql, this_dc, skip_without_tablets):
+ ksdef = f"WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 1}};"
+ with new_test_keyspace(cql, ksdef) as keyspace:
+ # preserving works when init_tablets is a positive int
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}}")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 1
+
+ # setting init_tablets to 0
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}} AND tablets = {{'initial': 0}};")
+
+ # preserving works when init_tablets is equal to 0
+ cql.execute(f"ALTER KEYSPACE {keyspace} WITH replication = {{'class': 'NetworkTopologyStrategy', '{this_dc}': 1}}")
+ res = cql.execute(f"SELECT * FROM system_schema.scylla_keyspaces WHERE keyspace_name = '{keyspace}'").one()
+ assert res.initial_tablets == 0
+
+
# 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' } " \

[Commit Bot]

From: Piotr Smaron <piotr....@scylladb.com>
Committer: Pavel Emelyanov <xe...@scylladb.com>

Branch: master

[Commit Bot]

From: Piotr Smaron <piotr....@scylladb.com>
Committer: Pavel Emelyanov <xe...@scylladb.com>

Branch: next-6.0

cql: forbid switching from tablets to vnodes in ALTER KS

This check is already in place, but isn't fully working, i.e.
switching from a vnode KS to a tablets KS is not allowed, but
this check doesn't work in the other direction. To fix the
latter, `ks_prop_defs::get_initial_tablets()` has been changed
to handle 3 states: (1) init_tablets is set, (2) it was skipped,
(3) tablets are disabled. These couldn't fit into std::optional,
so a new local struct to hold these states has been introduced.
Callers of this function have been adjusted to set init_tablets
to an appropriate value according to the circumstances, i.e. if
tablets are globally enabled, but have been skipped in the CQL,
init_tablets is automatically set to 0, but if someone executes
ALTER KS and doesn't provide tablets options, they're inherited
from the old KS.
I tried various approaches and this one resulted in the least
lines of code changed. I also provided testcases to explain how
the code behaves.

Fixes: #18795

(cherry picked from commit 758139c8b2dfe422dcd762615def177a715ac0e1)

Closes scylladb/scylladb#19540