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

kvserver: introduce methods on raftRecvQueue{,s} #80640

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

tbg
Copy link
Member

@tbg tbg commented Apr 27, 2022

This removes ad-hoc sync.Map-ish code from the various callers.
This commit also gives raftRecvQueue (note: no s) methods which
are now used instead of directly reaching into the struct.

Together this paves the way for tracking the size of the raft receive
queue. Since the sync.Map synchronization is very loose, before this
commit it was difficult to avoid leaking memory when queues were dropped
and accessed concurrently. And similarly, reaching directly into
individual queues will make it difficult to get things right.

Now there's more synchronization around dropping queues and correct
tracking of the sizes (& making sure there aren't leaks) is more
straightforward. (The actual tracking will be added in a follow-up).

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

This removes ad-hoc `sync.Map`-ish code from the various callers.
This commit also gives `raftRecvQueue` (note: no `s`) methods which
are now used instead of directly reaching into the struct.

Together this paves the way for tracking the size of the raft receive
queue.  Since the `sync.Map` synchronization is very loose, before this
commit it was difficult to avoid leaking memory when queues were dropped
and accessed concurrently. And similarly, reaching directly into
individual queues will make it difficult to get things right.

Now there's more synchronization around dropping queues and correct
tracking of the sizes (& making sure there aren't leaks) is more
straightforward. (The actual tracking will be added in a follow-up).

Release note: None
@tbg tbg marked this pull request as ready for review April 28, 2022 08:30
@tbg tbg requested a review from a team as a code owner April 28, 2022 08:30
@tbg tbg requested a review from erikgrinaker April 28, 2022 08:30
pkg/kv/kvserver/store_raft.go Show resolved Hide resolved
pkg/kv/kvserver/store_raft.go Show resolved Hide resolved
@tbg
Copy link
Member Author

tbg commented Apr 28, 2022

CI failure is probably not mine, #80696
Going to retrigger.

@tbg
Copy link
Member Author

tbg commented Apr 28, 2022

bors r=erikgrinaker
TFTR!

@craig
Copy link
Contributor

craig bot commented Apr 28, 2022

Build succeeded:

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.

3 participants