Skip to content

Conversation

@sumeerbhola
Copy link
Collaborator

This will be used in CockroachDB's loosely coupled raft log
truncation (see
https://github.com/cockroachdb/cockroach/blob/c5d146b536aaf70f0b91aa38f88d62b967ac8411/pkg/kv/kvserver/raft_log_queue.go#L959-L967
in a WIP PR).

This is done via a registration function instead of adding
to the Options passed to Open since it simplifies integration
in CockroachDB -- kvserver.NewStore is given an already open
DB.

This interface is subject to change, hence it is marked
experimental with a warning regarding usage.

@sumeerbhola sumeerbhola requested a review from a team February 9, 2022 22:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @sumeerbhola)

This will be used in CockroachDB's loosely coupled raft log
truncation (see
https://github.com/cockroachdb/cockroach/blob/c5d146b536aaf70f0b91aa38f88d62b967ac8411/pkg/kv/kvserver/raft_log_queue.go#L959-L967
in a WIP PR).

This is done via a registration function instead of adding
to the Options passed to Open since it simplifies integration
in CockroachDB -- kvserver.NewStore is given an already open
DB.

This interface is subject to change, hence it is marked
experimental with a warning regarding usage.
Copy link
Contributor

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @sumeerbhola)

@sumeerbhola
Copy link
Collaborator Author

TFTR!

@sumeerbhola sumeerbhola merged commit b2295ac into cockroachdb:master Feb 9, 2022
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: all files reviewed, 1 unresolved discussion


compaction.go, line 1353 at r2 (raw file):

//
// Only a single callback func can be registered. Repeated calls will replace
// the previous registered callback.

Rather than a new callback, could EventListener.FlushEnd provide the same signal?

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.

Reviewable status: all files reviewed, 1 unresolved discussion


compaction.go, line 1353 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Rather than a new callback, could EventListener.FlushEnd provide the same signal?

I missed this comment earlier. I think we could with an extra level of indirection in CockroachDB -- we won't have the real callback when creating the DB, so we will need to have the real callback be later registered in the Pebble struct.
It will be called while holding locks, but I don't think that is a big deal.

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.

Reviewable status: all files reviewed, 1 unresolved discussion


compaction.go, line 1353 at r2 (raw file):

Previously, sumeerbhola wrote…

I missed this comment earlier. I think we could with an extra level of indirection in CockroachDB -- we won't have the real callback when creating the DB, so we will need to have the real callback be later registered in the Pebble struct.
It will be called while holding locks, but I don't think that is a big deal.

I'll use FlushEnd in the forthcoming CockroachDB change, so I can rollback this change. Thanks for the suggestion.

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