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

Alternator: CreateTable + Tags should be atomic #6391

Closed
nyh opened this issue May 7, 2020 · 1 comment
Closed

Alternator: CreateTable + Tags should be atomic #6391

nyh opened this issue May 7, 2020 · 1 comment
Assignees
Labels
Milestone

Comments

@nyh
Copy link
Contributor

nyh commented May 7, 2020

The CreateTable operation has a Tags parameter used to set tags on the newly created table. However, the way we do this today is as two separate operations: first the table is created, then the tags are added. It is, at least in theory, possible for the table creation to succeed but then the tagging to fail - and the result is that the operation will seem to fail but still leave behind a new table, missing its tags.
If the tagging fails we should probably delete the newly-created table. Or even better, we should find a way to atomically create a table already already with its tags and not as two independent operations.

@nyh nyh added low area/alternator Alternator related Issues labels May 7, 2020
psarna pushed a commit that referenced this issue May 7, 2020
The "current compatibility with DynamoDB" section in alternator.md is where
we should list very briefly our state of compatibility - it's not the right
place to explain implementation details or track obscure bugs. I've
significantly shortened the "Tags" section because, in brief, we do
fully support tags and should say that we do.

I moved the two bugs mentioned in the text into the bug tracker:
Refs #6389
Refs #6391

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200507125022.22608-1-nyh@scylladb.com>
@slivne slivne added this to the 4.x milestone May 17, 2020
@nyh nyh self-assigned this Feb 3, 2022
denesb pushed a commit that referenced this issue Feb 10, 2022
This patch adds a "--raft" option to test/alternator/run to enable the
experimental Raft-based schema changes ("--experimental-features=raft")
when running Scylla for the tests. This is the same option we added to
test/cql-pytest/run in a previous patch.

Note that we still don't have any Alternator tests that pass or fail
differently in these two modes - these will probably come later as we
fix issues #9868 and #6391. But in order to work on fixing those issues
we need to be able to run the tests in Raft mode.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220209123144.321344-1-nyh@scylladb.com>
denesb pushed a commit that referenced this issue Feb 14, 2022
We add reproducing tests for two known Alternator issues, #6391 and #9868,
which involve the non-atomicity of table creation. Creating a table
currently involves multiple steps - creating a keyspace, a table,
materialized views, and tags. If some of these steps succeed and some
fail, we get an InternalServerError and potentially leave behind some
half-built table.

Both issues will be solved by making better use of the new Raft-based
capabilities of making multiple modifications to the schema atomically,
but this patch doesn't fix the problem - it just proves it exist.

The new tests involve two threads - one repeatedly trying to create a
table with a GSI or with tags - and the other thread repeatedly trying
to delete the same table under its feet. Both bugs are reproduced almost
immediately.

Note that like all test/alternator tests, the new tests are usually run on
just one node. So when we fix the bug and these tests begin to pass,
it will not be a proof that concurrent schema modification works safely
on *different* nodes. To prove that, we will also need a multi-node test.
However, this test can prove that we used Raft-based schema modification
correctly - and if we assume that the Raft-based schema modification
feature is itself correct, then we can be sure that CreateTable will be
correct also across multiple nodes. Although it won't hurt to check it
directly.

Refs #6391
Refs #9868

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220207223100.207074-1-nyh@scylladb.com>
nyh added a commit to nyh/scylla that referenced this issue Feb 14, 2022
We add reproducing tests for two known Alternator issues, scylladb#6391 and scylladb#9868,
which involve the non-atomicity of table creation. Creating a table
currently involves multiple steps - creating a keyspace, a table,
materialized views, and tags. If some of these steps succeed and some
fail, we get an InternalServerError and potentially leave behind some
half-built table.

Both issues will be solved by making better use of the new Raft-based
capabilities of making multiple modifications to the schema atomically,
but this patch doesn't fix the problem - it just proves it exist.

The new tests involve two threads - one repeatedly trying to create a
table with a GSI or with tags - and the other thread repeatedly trying
to delete the same table under its feet. Both bugs are reproduced almost
immediately.

Note that like all test/alternator tests, the new tests are usually run on
just one node. So when we fix the bug and these tests begin to pass,
it will not be a proof that concurrent schema modification works safely
on *different* nodes. To prove that, we will also need a multi-node test.
However, this test can prove that we used Raft-based schema modification
correctly - and if we assume that the Raft-based schema modification
feature is itself correct, then we can be sure that CreateTable will be
correct also across multiple nodes. Although it won't hurt to check it
directly.

Refs scylladb#6391
Refs scylladb#9868

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@DoronArazii DoronArazii modified the milestones: 5.x, 5.0 Jul 14, 2022
@DoronArazii DoronArazii modified the milestones: 5.0, 5.1 Nov 8, 2022
@nyh
Copy link
Contributor Author

nyh commented Dec 4, 2022

Already in branch 5.1. Not backporting to 5.0 because it relies on the new experimental Raft-based schema changes reorganization of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants