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

db: add implementation warning about requirement imposed by SingleDelete #4343

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

sumeerbhola
Copy link
Collaborator

No description provided.

@sumeerbhola sumeerbhola requested a review from a team as a code owner February 14, 2025 22:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @pav-kv and @petermattis)

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @pav-kv)


db.go line 154 at r1 (raw file):

	// flushable are usually in one-to-one correspondence, and atomically
	// updating the MANIFEST when the flushable is flushed (which ensures the
	// WAL will never be replayed). There is one exception: a flushableBatch is

Might want to add some words to explain what a flushableBatch is. a flushableBatch (a batch too large to fit in a memtable) ....

The logical memtable splitting idea would also violate the 1:1 correspondence between a flushable and WAL. If we decide to precede with that proposal I think we should find a way to refactor the code to more clearly indicate that a contiguous span of flushables needs to be flushed together. It is subtle the way flushableBatch.readyToFlush() achieves this. I'm pretty sure I'm to blame for that.

Cc @RaduBerinde as he often has good ideas for cleaner implementations.

@sumeerbhola sumeerbhola force-pushed the singledel_impl_comment branch from 1463838 to 1465ddc Compare February 18, 2025 16:13
Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

TFTRs!

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @pav-kv and @RaduBerinde)


db.go line 154 at r1 (raw file):

Might want to add some words to explain what a flushableBatch is. a flushableBatch (a batch too large to fit in a memtable) ....

Done

The logical memtable splitting idea would also violate the 1:1 correspondence between a flushable and WAL. If we decide to precede with that proposal I think we should find a way to refactor the code to more clearly indicate that a contiguous span of flushables needs to be flushed together. It is subtle the way flushableBatch.readyToFlush() achieves this. I'm pretty sure I'm to blame for that.

Ack

@sumeerbhola sumeerbhola merged commit 6949b90 into cockroachdb:master Feb 18, 2025
22 of 23 checks passed
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.

4 participants