-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Compact: Make GatherNoCompactionMarkFilter.NoCompactMarkedBlocks concurrency safe #5736
Compact: Make GatherNoCompactionMarkFilter.NoCompactMarkedBlocks concurrency safe #5736
Conversation
2b83136
to
aed26bd
Compare
…urrency safe This fixes a crash due to concurrent access. concurrent map read and map write fixes thanos-io#5735 Signed-off-by: Igor Wiedler <iwiedler@gitlab.com>
aed26bd
to
860e868
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too familiar with the compactor, but this looks safe to me. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well, thank you!
@@ -1361,6 +1361,7 @@ type GatherNoCompactionMarkFilter struct { | |||
bkt objstore.InstrumentedBucketReader | |||
noCompactMarkedMap map[ulid.ULID]*metadata.NoCompactMark | |||
concurrency int | |||
mtx sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better using rw mutex?
This fixes a crash due to concurrent access.
fixes #5735
Changes
GatherNoCompactionMarkFilter.NoCompactMarkedBlocks
Verification
Let's see what CI says first.