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

kv: remove ShouldWinOriginTimestampTie option fromcput #143096

Merged

Conversation

jeffswenson
Copy link
Collaborator

The current KV writer does not use ShouldWinOriginTimestampTie, so it can be removed as unused code. The KV LDR writer stopped using this approach for tie-breaking due to two issues:

  1. When replaying replicated writes, they win LWW against themselves, leading to duplicate KVs. This is inefficient, slowing performance and wasting storage until garbage collection (GC) removes the duplicates.
  2. The approach fails to handle semantics correctly in three-way replication scenarios.

Release note: none
Epic: CRDB-48647

The current KV writer does not use ShouldWinOriginTimestampTie, so it
can be removed as unused code. The KV LDR writer stopped using this
approach for tie-breaking due to two issues:

1. When replaying replicated writes, they win LWW against themselves,
	 leading to duplicate KVs. This is inefficient, slowing performance
	 and wasting storage until garbage collection (GC) removes the
	 duplicates.
2. The approach fails to handle semantics correctly in three-way
	 replication scenarios.

Release note: none
Epic: CRDB-48647
@jeffswenson jeffswenson requested a review from msbutler March 18, 2025 21:47
@jeffswenson jeffswenson requested review from a team as code owners March 18, 2025 21:47
@jeffswenson jeffswenson requested review from jbowens, DrewKimball and stevendanna and removed request for a team March 18, 2025 21:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

👍 on just removing this until we can think through how we want to handle ties in the 3-way replication case a bit more.

I wonder if ties are more common on macOS where we don't have nanosecond-precise timestamps.

@jeffswenson
Copy link
Collaborator Author

Thanks for the review!

👍 on just removing this until we can think through how we want to handle ties in the 3-way replication case a bit more.

The way I think we should handle this is to pick unique replication cluster ids for each LDR peer (instead of 1 == remote). Then LDR would supply the ID of the local cluster and the replicating cluster. The only adjustment we need for tie handling is a cluster should lose ties against itself to avoid writing a row multiple times.

An alternative strategy is we could make the value encoding canonical and use the value to break ties. This would also make the cput based optimizations more efficient.

bors r+

@craig craig bot merged commit 8c96475 into cockroachdb:master Mar 19, 2025
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.

4 participants