Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cql: reject ALTER tablets KS with repl opts #18772

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cql3/statements/alter_keyspace_statement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ void cql3::statements::alter_keyspace_statement::validate(query_processor& qp, c
}
try {
auto ks = qp.db().find_keyspace(_name);
// TODO: the full tablets support will be implemented in https://github.com/scylladb/scylladb/issues/16129
if (ks.get_replication_strategy().uses_tablets() && !_attrs->get_replication_options().empty()) {
throw exceptions::invalid_request_exception("Changing replication options via ALTER KEYSPACE statement is not yet supported for keyspaces with tablets.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that recently promoted #18899 is prerequisite to this restriction, isn't it?
Maybe it's worth moving this one lower, after the

auto new_ks = _attrs->as_ks_metadata_update(ks.metadata(), *qp.proxy().get_token_metadata_ptr(), qp.proxy().features());

is done, and then later we can relax the restriction to be

    if (ks.uses_tablets() && new_ks->strategy_options().options != ks.strategy_options().options) {
        throw invalid_request_exception("Changing replication options is not supported");
    }

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptrsmrn , you put 👍 and resolved the issue, but the code remained as it was before my suggestion, so I'm totally confused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xemul sorry, I just want to move quick, maybe I'm too quick and cut corners:

I assume that recently promoted #18899 is prerequisite to this restriction, isn't it?

Yes, it's a prerequisite.

As for the 2nd part of your comment, I assumed it's only about refactoring, and since I didn't want to refactor the code, build, test, push, re-run the CI (not to waste time), I ignored it.

data_dictionary::storage_options current_options = ks.metadata()->get_storage_options();
data_dictionary::storage_options new_options = _attrs->get_storage_options();
if (!qp.proxy().features().keyspace_storage_options && !new_options.is_local_type()) {
Expand Down
11 changes: 11 additions & 0 deletions docs/cql/ddl.rst
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,17 @@ For instance::

The supported options are the same as :ref:`creating a keyspace <create-keyspace-statement>`.

ALTER KEYSPACE with Tablets :label-caution:`Experimental`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Modifying a keyspace with tablets enabled is not yet fully supported and all ``ALTER KEYSPACE`` statements specifying any replication option issued on a keyspace with tablets enabled are going to be rejected:

.. code-block:: cql

cqlsh> CREATE KEYSPACE excalibur WITH replication = {'class': 'NetworkTopologyStrategy', 'DC1': 4} and tablets = {'initial': 8};
cqlsh> ALTER KEYSPACE excalibur WITH replication = {'class': 'NetworkTopologyStrategy', 'DC1': 3};
InvalidRequest: Error from server: code=2200 [Invalid query] message="Changing replication options via ALTER KEYSPACE statement is not yet supported for keyspaces with tablets."

.. _drop-keyspace-statement:

DROP KEYSPACE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ def testAlterKeyspaceWithNoOptionThrowsConfigurationException(cql, test_keyspace

# Test {@link ConfigurationException} thrown when altering a keyspace to
# invalid DC option in replication configuration.
@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def testAlterKeyspaceWithNTSOnlyAcceptsConfiguredDataCenterNames(cql, test_keyspace, this_dc):
# Create a keyspace with expected DC name.
with create_keyspace(cql, "replication={ 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 2 }") as ks:
Expand All @@ -231,6 +232,7 @@ def testAlterKeyspaceWithNTSOnlyAcceptsConfiguredDataCenterNames(cql, test_keysp
# Mix valid and invalid, should throw an exception
assert_invalid_throw_message_re(cql, ks, "Unrecognized strategy option {INVALID_DC} passed to .*NetworkTopologyStrategy for keyspace " + ks, ConfigurationException, "ALTER KEYSPACE %s WITH replication={ 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 2 , 'INVALID_DC': 1}")

@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def testAlterKeyspaceWithMultipleInstancesOfSameDCThrowsSyntaxException(cql, test_keyspace, this_dc):
# Create a keyspace
with create_keyspace(cql, "replication={ 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 2 }") as ks:
Expand Down
7 changes: 7 additions & 0 deletions test/cql-pytest/test_keyspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,17 @@ def test_drop_keyspace_nonexistent(cql):
cql.execute('DROP KEYSPACE nonexistent_keyspace')

# Test trying to ALTER a keyspace.
@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def test_alter_keyspace(cql, this_dc):
with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 1 }") as keyspace:
cql.execute(f"ALTER KEYSPACE {keyspace} WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', '{this_dc}' : 3 }} AND DURABLE_WRITES = false")

def test_alter_keyspace_without_providing_replication_options(cql, this_dc):
with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 1 }") as keyspace:
cql.execute(f"ALTER KEYSPACE {keyspace} WITH DURABLE_WRITES = false")

# Test trying to ALTER a keyspace with invalid options.
@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def test_alter_keyspace_invalid(cql, this_dc):
with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 1 }") as keyspace:
with pytest.raises(ConfigurationException):
Expand Down Expand Up @@ -130,6 +136,7 @@ def test_alter_keyspace_missing_rf(cql, this_dc, scylla_only, has_tablets):

# Test trying to ALTER a keyspace with invalid options.
# Reproduces #7595.
@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def test_alter_keyspace_nonexistent_dc(cql, this_dc):
with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 1 }") as keyspace:
with pytest.raises(ConfigurationException):
Expand Down
1 change: 1 addition & 0 deletions test/cql-pytest/test_tablets.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ def test_tablets_can_be_explicitly_disabled(cql, skip_without_tablets):
assert len(list(res)) == 0, "tablets replication strategy turned on"


@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
def test_alter_changes_initial_tablets(cql, skip_without_tablets):
ksdef = "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1} AND tablets = {'initial': 1};"
with new_test_keyspace(cql, ksdef) as keyspace:
Expand Down
1 change: 1 addition & 0 deletions test/topology_custom/test_tablets.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@


@pytest.mark.asyncio
@pytest.mark.xfail(reason="Issue #16129; ALTER tablets KS doesn't support replication options")
async def test_tablet_replication_factor_enough_nodes(manager: ManagerClient):
cfg = {'enable_user_defined_functions': False,
'experimental_features': ['tablets']}
Expand Down
Loading