-
Notifications
You must be signed in to change notification settings - Fork 514
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
Prevent data race by using xxhash instance as a local variable #1387
Conversation
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.
PR is looking good. Thanks for the contribution!
If you can add a bugfix changelog entry we will merge.
Thank you. Added the change log entry. |
tempodb/encoding/v2/appender.go
Outdated
a.hash.Reset() | ||
_, _ = a.hash.Write(id) | ||
hash := a.hash.Sum64() | ||
hasher := xxhash.New() |
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.
fwiw - I think we could keep the static hash for writes and just alloc a new one for reads.
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.
yeah, that's fair. it's a small improvement, but this code is called constantly.
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.
I have updated the code
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.
Nice fix! Thanks
What this PR does:
A shared instance of xxhash causes a data race and crash in the ingester. This PR fixes by converting the shared instance to a local variable.
Which issue(s) this PR fixes:
Fixes #1384
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]