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

Fix race condition when updating lastSeriesID during loading chunk snapshot #11099

Merged
merged 1 commit into from Aug 4, 2022

Conversation

damnever
Copy link
Contributor

@damnever damnever commented Aug 3, 2022

No description provided.

@damnever damnever requested a review from codesome as a code owner August 3, 2022 02:54
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Nice catch!

@codesome
Copy link
Member

codesome commented Aug 3, 2022

Can you please fix the DCO? Thanks!

@roidelapluie
Copy link
Member

What is the impact of this pull request? Should it have a test?

@damnever
Copy link
Contributor Author

damnever commented Aug 3, 2022

@codesome DCO fixed

@roidelapluie lastSeriesID is used to generate a new id for new series, the problem is this race condition may cause lastSeriesID less than the maximum we know, thus multiple different series may get the same id.

…apshot

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>
@roidelapluie
Copy link
Member

I think a test is relevant. I also wonder what are the chances that this creates an infinite loop in the wild.

@damnever
Copy link
Contributor Author

damnever commented Aug 4, 2022

Maybe we should do some checks at the runtime since this kind of issue is severe.

I also wonder what are the chances that this creates an infinite loop in the wild.

I don't think this is an issue since the operation is fast and the runtime shouldn't schedule it away if we are in the CAS loop.

@codesome
Copy link
Member

codesome commented Aug 4, 2022

This might be hard to reproduce in a test. And I don't think we need a test to verify infinite loop, it will be limited to the number of series. I will merge this for now so that we can get it in the next release, and in the meantime we can see if we can write a test for this.

@codesome codesome merged commit 1078081 into prometheus:main Aug 4, 2022
Mama59 pushed a commit to Arnoways/prometheus that referenced this pull request Aug 25, 2022
…apshot (prometheus#11099)

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>
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.

None yet

3 participants