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

cdc: ensure that CDC generation write is flushed to commitlog before ack #7619

Closed
wants to merge 1 commit into from

Conversation

kbr-
Copy link
Contributor

@kbr- kbr- commented Nov 16, 2020

When a node bootstraps or upgrades from a pre-CDC version, it creates a
new CDC generation, writes it to a distributed table
(system_distributed.cdc_generation_descriptions), and starts gossiping
its timestamp. When other nodes see the timestamp being gossiped, they
retrieve the generation from the table.

The bootstrapping/upgrading node therefore assumes that the generation
is made durable and other nodes will be able to retrieve it from the
table. This assumption could be invalidated if periodic commitlog mode
was used: replicas would acknowledge the write and then immediately
crash, losing the write if they were unlucky (i.e. commitlog wasn't
synced to disk before the write was acknowledged).

This commit enforces all writes to the generations table to be
synced to commitlog immediately. It does not matter for performance as
these writes are very rare.

Fixes #7610.

Tested by inserting a tactical sleep in commitlog code which allowed me to reproduce issue #7610 with each run of the test_periodic_commitlog test. With this commit the bug no longer occurred.

When a node bootstraps or upgrades from a pre-CDC version, it creates a
new CDC generation, writes it to a distributed table
(system_distributed.cdc_generation_descriptions), and starts gossiping
its timestamp. When other nodes see the timestamp being gossiped, they
retrieve the generation from the table.

The bootstrapping/upgrading node therefore assumes that the generation
is made durable and other nodes will be able to retrieve it from the
table. This assumption could be invalidated if periodic commitlog mode
was used: replicas would acknowledge the write and then immediately
crash, losing the write if they were unlucky (i.e. commitlog wasn't
synced to disk before the write was acknowledged).

This commit enforces all writes to the generations table to be
synced to commitlog immediately. It does not matter for performance as
these writes are very rare.

Fixes scylladb#7610.
Copy link
Contributor

@haaawk haaawk left a comment

Choose a reason for hiding this comment

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

LGTM

@haaawk
Copy link
Contributor

haaawk commented Nov 16, 2020

Have you checked the dtest stops failing with this fix @kbr- ?

@kbr-
Copy link
Contributor Author

kbr- commented Nov 16, 2020

Yes. I added a paragraph at the end of the PR letter which mentions it (refresh github if it doesn't show it).

Copy link
Contributor

@nyh nyh left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll commit.
@gleb-cloudius I'm worried that this is a slippery slope - that no that "extra durability" is available, everyone would want it. Who will settle with "durability" when "extra durability" is available? And this is an example. But I understood during today's status call (I hope I'm not mistaken) that Gleb is aware of this new use and aproves of it.

@gleb-cloudius
Copy link
Contributor

Don't you need to add a call to builder.set_wait_for_sync_to_commitlog(true) to db/system_distributed_keyspace.cc:cdc_generations() for the case where it is created for the first time?

@kbr-
Copy link
Contributor Author

kbr- commented Nov 17, 2020

@gleb-cloudius it looks like it's not needed, the schema object that is used in the end when writing to the table is the one returned from create_table_from_mutations (the one defined in cdc_generations() is used only to announce the mutations, but this flag is not a part of the mutations, every node decides locally if the table should be extra durable).

avikivity pushed a commit that referenced this pull request Nov 17, 2020
When a node bootstraps or upgrades from a pre-CDC version, it creates a
new CDC generation, writes it to a distributed table
(system_distributed.cdc_generation_descriptions), and starts gossiping
its timestamp. When other nodes see the timestamp being gossiped, they
retrieve the generation from the table.

The bootstrapping/upgrading node therefore assumes that the generation
is made durable and other nodes will be able to retrieve it from the
table. This assumption could be invalidated if periodic commitlog mode
was used: replicas would acknowledge the write and then immediately
crash, losing the write if they were unlucky (i.e. commitlog wasn't
synced to disk before the write was acknowledged).

This commit enforces all writes to the generations table to be
synced to commitlog immediately. It does not matter for performance as
these writes are very rare.

Fixes #7610.

Closes #7619

(cherry picked from commit d74f303)
@avikivity
Copy link
Member

Backported to 4.3.

@kbr-
Copy link
Contributor Author

kbr- commented Nov 21, 2020

@avikivity could we also backport this to 4.2?

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

Successfully merging this pull request may close these issues.

6 participants