tablets, mv: create tablets for a new materialized view#16205
Closed
nyh wants to merge 1 commit intoscylladb:masterfrom
Closed
tablets, mv: create tablets for a new materialized view#16205nyh wants to merge 1 commit intoscylladb:masterfrom
nyh wants to merge 1 commit intoscylladb:masterfrom
Conversation
Before this patch, trying to create a materialized view when tablets are enabled for a keyspace results in a failure: "Tablet map not found for table <uuid>", with uuid referring to the new view. When a table schema is created, the handler on_before_create_column_family() is called - and this function creates the tablet map for the new table. The bug was that we forgot to do the same when creating a materialized view - which also a bona-fide table. In this patch we call on_before_create_column_family() also when creating the materialized view. I decided *not* to create a new callback (e.g., on_before_create_view()) and rather call the existing on_before_create_column_family() callback - after all, a view is a column family too. This patch also includes a test for this issue, which fails to create the view before this patch, and passes with the patch. The test is in the test/topology_experimental_raft suite, which runs Scylla with the tablets experimental feature, and will also allow me to create tests that need multiple nodes. However, the first test added here only needs a single node to reproduce the bug and validate its fix. Fixes scylladb#16194. Signed-off-by: Nadav Har'El <[email protected]>
avikivity
reviewed
Nov 28, 2023
| mlogger.info("Create new view: {}", view); | ||
| auto mutations = db::schema_tables::make_create_view_mutations(keyspace, std::move(view), ts); | ||
| co_return co_await include_keyspace(sp, *keyspace, std::move(mutations)); | ||
| return seastar::async([&db, keyspace = std::move(keyspace), &sp, view = std::move(view), ts] { |
Member
There was a problem hiding this comment.
Why are you de-cooroutinizing this function?
Contributor
Author
There was a problem hiding this comment.
Because I had to add seastar::async (I forgot to explain this in the commit message - the on_* callbacks all require to be run in a Seastar thread). Since async() returns a future, it's easier to just return this future and be done with it, no need to do the "co_return co_await" song and dance. The only other co_return in this function was an exception return, and also was only made more complicated by by the coroutinization.
Contributor
🟢 CI State: SUCCESS✅ - Build Build Details:
|
tgrabiec
approved these changes
Nov 28, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this patch, trying to create a materialized view when tablets are enabled for a keyspace results in a failure: "Tablet map not found for table ", with uuid referring to the new view.
When a table schema is created, the handler on_before_create_column_family() is called - and this function creates the tablet map for the new table. The bug was that we forgot to do the same when creating a materialized view - which also a bona-fide table.
In this patch we call on_before_create_column_family() also when creating the materialized view. I decided not to create a new callback (e.g., on_before_create_view()) and rather call the existing on_before_create_column_family() callback - after all, a view is a column family too.
This patch also includes a test for this issue, which fails to create the view before this patch, and passes with the patch. The test is in the test/topology_experimental_raft suite, which runs Scylla with the tablets experimental feature, and will also allow me to create tests that need multiple nodes. However, the first test added here only needs a single node to reproduce the bug and validate its fix.
Fixes #16194.