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

crosscluster: handle lww condition failures #142368

Merged
merged 1 commit into from
Mar 22, 2025

Conversation

jeffswenson
Copy link
Collaborator

@jeffswenson jeffswenson commented Mar 5, 2025

This change aims to correct the SQL writer's LWW semantics, enabling the
retirement of the KV writer. If the SQL writer observes the condition
failed error's introduced by #143100, it will drop the row as a LWW
loss. This closes a LWW bug where an old replicated write can overwrite
a recent local tombstone.

The first two commits in this PR are from:

Release note: none
Epic: CRDB-48647

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch from 9b9d88c to 4a3877f Compare March 6, 2025 15:27
@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch 13 times, most recently from d8c4119 to 434bfc7 Compare March 18, 2025 18:11
@jeffswenson jeffswenson marked this pull request as ready for review March 18, 2025 18:11
@jeffswenson jeffswenson requested review from a team as code owners March 18, 2025 18:11
@jeffswenson jeffswenson requested review from RaduBerinde, kev-cao, yuzefovich, stevendanna, michae2 and msbutler and removed request for a team March 18, 2025 18:11
@jeffswenson
Copy link
Collaborator Author

This PR contains the KV changes required for https://docs.google.com/document/d/1rdt5IhmPPkTxnExfuDRDtnXN4OFca78bIGKwO4SRK5w/edit?usp=sharing. Once this merges, the only remaining LWW bugs in the SQL writer will be:

  1. The delete-delete race case that requires an explicit cput to update the tombstone.
  2. Tie handling. Which is also slightly wrong in the KV writer.

@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch from 434bfc7 to be98d04 Compare March 18, 2025 20:28
@msbutler
Copy link
Collaborator

msbutler commented Mar 18, 2025

@jeffswenson one annoying nit: in general, the convention is to break up 1 large commit that spans multiple packages into smaller, package scoped commits, especially if the commit has reviewers from several teams. Batching multiple commits in one PR is totally fine.

@jeffswenson
Copy link
Collaborator Author

@jeffswenson one annoying nit: in general, the convention is to break up 1 large commit that spans multiple packages into smaller, package scoped commits, especially if the commit has reviewers from several teams. Batching multiple commits in one PR is totally fine.

Fair point. I'll start breaking this up into multiple PRs. Here's a small PR that removes the tie break option #143096.

@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch from be98d04 to 73a79c1 Compare March 18, 2025 21:58
jeffswenson added a commit to jeffswenson/cockroach that referenced this pull request Mar 18, 2025

Verified

This commit was signed with the committer’s verified signature.
bdemers Brian Demers
This change aims to correct the SQL writer's LWW semantics, enabling the
retirement of the KV writer. If the SQL writer observes the condition
failed error's introduced by cockroachdb#142368, it will drop the row as a LWW
loss. This closes a LWW bug where an old replicated write can overwrite
a recent local tombstone.

Release note: none
Epic: CRDB-48647
@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch from 73a79c1 to 136b1c9 Compare March 18, 2025 22:44
@jeffswenson
Copy link
Collaborator Author

I split this change up into three stacked PRs.

@jeffswenson jeffswenson changed the title crosscluster: use new origin header values crosscluster: handle lww condition failures Mar 18, 2025
@yuzefovich yuzefovich removed their request for review March 18, 2025 23:44
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

3rd commit LGTM!

Comment on lines -462 to -464
if !useKVProc {
skip.IgnoreLint(t, "local delete ordering is not handled correctly by the SQL processor")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

This change aims to correct the SQL writer's LWW semantics, enabling the
retirement of the KV writer. If the SQL writer observes the condition
failed error's introduced by cockroachdb#142368, it will drop the row as a LWW
loss. This closes a LWW bug where an old replicated write can overwrite
a recent local tombstone.

Release note: none
Epic: CRDB-48647
@jeffswenson jeffswenson force-pushed the jeffswenson-ldr-always-origin branch from 136b1c9 to 7193813 Compare March 22, 2025 12:53
@jeffswenson
Copy link
Collaborator Author

Thanks for the reviews!

bors r+

@craig craig bot merged commit a72d15b into cockroachdb:master Mar 22, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants