Skip to content

[WIP] Remove userStatesMtx #859

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[WIP] Remove userStatesMtx #859

wants to merge 1 commit into from

Conversation

bboreham
Copy link
Contributor

Fixes #662. This bugs me every time I read that code.

@bboreham
Remove userStatesMtx
4bb5a30 
It is read-locked in lots of places, but only ever write-locked for a
brief time after receiving transferred chunks.  This behaviour is not
required, and it is confusing.
@rade
Copy link

rade commented Jun 29, 2018

AFAIK, golang pointer assignment is not guaranteed to be atomic.

@rade
Copy link

rade commented Jun 29, 2018

There are pointer operations in sync/atomic that could replace the mutex.

@bboreham
Copy link
Contributor Author

bboreham commented Jun 29, 2018 •
edited
Loading

OK, it's plausible that's what it's for, but if that were the case it shouldn't lock round the ChangeState() too.

@bboreham
Copy link
Contributor Author

actually that probably is intentional, but could be fixed by doing the update to userStates before ChangeState()

@bboreham
Copy link
Contributor Author

But why is this pointer write happening anyway? Why not just write the newly-received chunks into the existing userStates? What are we protecting against?

@tomwilkie
Copy link
Contributor

But why is this pointer write happening anyway? Why not just write the newly-received chunks into the existing userStates? What are we protecting against?

I believe I was protecting against incoming writes creating new series/chunks, but the ingester state should prevent this itself.

@tomwilkie
Copy link
Contributor

You could add a swap method to userStates that use the mtx in userstates to swap out the underlying map.

@bboreham bboreham changed the title Remove userStatesMtx [WIP] Remove userStatesMtx Jul 2, 2018
@tomwilkie tomwilkie mentioned this pull request Sep 20, 2018
Closed
@tomwilkie
Copy link
Contributor

Do you think we should close this PR?

@tomwilkie
Copy link
Contributor

Do you think we should close this PR?

I'm gonna say yes now.

@tomwilkie tomwilkie closed this Nov 29, 2018
@tomwilkie tomwilkie deleted the remove-userstatesmtx branch January 3, 2019 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bboreham @rade @tomwilkie