bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

cql: change validating NetworkTopologyStrategy tags to internal_error

Browse Source 
The check for `replication_factor` tag in
`network_topology_strategy::validate_options` is redundant for 2 reasons:
- before we reach this part of the code, the `replication_factor` tag
  is replaced with specific DC names
- we actually do allow for `replication_factor` tag in
  NetworkTopologyStrategy for keyspaces that have tablets disabled.

This code is unreachable, hence changing it to an internal error, which
means this situation should never occur.
The place that unrolls `replication_factor` tag checked for presence of
this tag ignoring the case, which lead to an unexpected behaviour:
- `replication_factor` tag (note the lowercase) was unrolled, as
  explained above,
- the same tag but written in any other case resulted in throwing a vague
  message: "replication_factor is an option for SimpleStrategy, not
NetworkTopologyStrategy".

So we're changing this validation to accept and unroll only the
lowercase version of this tag. We can't ignore the case here, as this
tag is present inside a json, and json is case-sensitive, even though the
CQL itself is case insensitive.

Added a test that passes for both scylla and cassandra.

Fixes: #15336
This commit is contained in:
Piotr Smaron
2024-08-16 20:16:58 +02:00
parent 2a77405093
commit 100e8d2856
2 changed files with 21 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
locator/network_topology_strategy.cc
View File

@@ -58,9 +58,12 @@ network_topology_strategy::network_topology_strategy(replication_strategy_params
}
if (boost::iequals(key, "replication_factor")) {
throw exceptions::configuration_exception(
"replication_factor is an option for SimpleStrategy, not "
"NetworkTopologyStrategy");
if (boost::equals(key, "replication_factor")) {
on_internal_error(rslogger, "replication_factor should have been replaced with a DC:RF mapping by now");
} else {
throw exceptions::configuration_exception(format(
"'{}' is not a valid option, did you mean (lowercase) 'replication_factor'?", key));
}
}
auto rf = parse_replication_factor(val);
@@ -274,9 +277,8 @@ void network_topology_strategy::validate_options(const gms::feature_service& fs)
validate_tablet_options(*this, fs, _config_options);
for (auto& c : _config_options) {
if (c.first == sstring("replication_factor")) {
throw exceptions::configuration_exception(
"replication_factor is an option for simple_strategy, not "
"network_topology_strategy");
on_internal_error(rslogger, format("'replication_factor' tag should be unrolled into a list of DC:RF by now."
"_config_options:{}", _config_options));
}
parse_replication_factor(c.second);
}

13
test/cqlpy/test_keyspace.py
View File

@@ -95,6 +95,19 @@ def test_create_keyspace_double_with(cql):
with pytest.raises(SyntaxException):
cql.execute('CREATE KEYSPACE ks WITH WITH DURABLE_WRITES = true')
# Scylla accepts 'replication_factor' in CREATE KEYSPACE statement,
# but while CQL syntax is case-insensitive, tags within json are case-sensitive,
# hence anything else than the lowercase 'replication_factor' should be rejected.
# Reproduces #15336
def test_create_keyspace_with_case_sensitive_replication_factor_tag(cql):
ks = unique_name()
# lowercase 'replication_factor' should be accepted
with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 3 }"):
pass
# 'replication_factor' in any other case than the lowercase should be rejected
with pytest.raises(ConfigurationException):
cql.execute(f"CREATE KEYSPACE {ks} WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', 'Replication_factor' : 3 }}")
# Test trying a non-existent keyspace - with or without the IF EXISTS flag.
# This test demonstrates a change of the exception produced between Cassandra 4.0
# and earlier versions (with Scylla behaving like the earlier versions).