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

added function for histograms merge #73

Merged

Conversation

AndrewChubatiuk
Copy link
Contributor

Added function to merge histograms

h.mu.Lock()
defer h.mu.Unlock()

b.mu.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

does Lock cover RLock as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://pkg.go.dev/sync#RWMutex.Lock

Lock locks rw for writing. If the lock is already locked for reading or writing, Lock blocks until the lock is available.

so just Lock is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there're locks on different objects - write lock on a target histogram and read lock on src histogram

Copy link
Contributor

@tenmozes tenmozes May 19, 2024

Choose a reason for hiding this comment

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

oh sorry, makes sense ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

potentially, it may cause deadlock.

If goroutine 1 calls merge at histogram A for histogram B and goroutine 2 calls merge at histogram B for histogram A.

Copy link
Contributor

@f41gh7 f41gh7 left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewChubatiuk AndrewChubatiuk merged commit a52bdc6 into VictoriaMetrics:master May 20, 2024
1 check passed
@AndrewChubatiuk AndrewChubatiuk deleted the merge-histograms branch May 20, 2024 10:52
valyala added a commit that referenced this pull request Jul 16, 2024
The Histogram.Merge() function is needed for VictoriaMetrics/VictoriaMetrics#6314

Updates #73
@valyala
Copy link
Contributor

valyala commented Jul 16, 2024

@AndrewChubatiuk , @f41gh7 , @tenmozes , please see the follow-up commit - 7a44715

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

4 participants