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.

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

fix leaking notifier in ruler when user is removed #4718

Merged
merged 15 commits into from
Jun 16, 2022
Merged

fix leaking notifier in ruler when user is removed #4718

merged 15 commits into from
Jun 16, 2022

Conversation

roystchiang
Copy link
Contributor

@roystchiang roystchiang commented Apr 14, 2022 •
edited
Loading

What this PR does:

Stops notifier when a user is removed from ruler.

Currently if a user is removed from ruler, the notifier keeps on accumulating. If a cluster has high churn, memory will grow at a faster pace

Screen Shot 2022-04-13 at 7 57 22 PM

Which issue(s) this PR fixes:

Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@roystchiang roystchiang marked this pull request as draft April 14, 2022 02:07
alvinlin123
alvinlin123 reviewed Apr 14, 2022
View reviewed changes
@roystchiang roystchiang force-pushed the fix-ruler-leak branch 4 times, most recently from 27d1a37 to ea2db88 Compare April 14, 2022 03:04
@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 14, 2022
@roystchiang roystchiang force-pushed the fix-ruler-leak branch 2 times, most recently from 38d2d25 to a2726ff Compare April 14, 2022 05:37
@rohang98 rohang98 force-pushed the fix-ruler-leak branch 2 times, most recently from 39fbbbc to c1e43ec Compare April 26, 2022 18:23
@rohang98
Fixed changelog
0fbbecb 
Signed-off-by: Rohan Gupta <[email protected]>
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 26, 2022
@rohang98
Fixed integ tests
15c7978 
Signed-off-by: Rohan Gupta <[email protected]>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 27, 2022
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 27, 2022
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 9, 2022
@pull-request-size pull-request-size bot added size/S and removed size/M labels May 31, 2022
@alvinlin123
merge from upstream master
3cc19b7 
Signed-off-by: Alvin Lin <[email protected]>
@alvinlin123
Revert "fix integration test failure"
1c85ef2 
This reverts commit 7387f2d.

Signed-off-by: Alvin Lin <[email protected]>
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 31, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 1, 2022
@alvinlin123
Copy link
Contributor

Although all the checks passed, I think we still need update manager_metrics_test.go's populateManager() function to take the new metrics I added into manager_metrics.go into consideration.

@roystchiang
update unit tests
6795b07 
Signed-off-by: Roy Chiang <[email protected]>
@alvinlin123 alvinlin123 marked this pull request as ready for review June 10, 2022 00:21
@alvinlin123
Copy link
Contributor

Thanks for adding the test @roystchiang. I've mark this PR Ready for Review.

@alvinlin123 alvinlin123 requested a review from alanprot June 10, 2022 00:33
@alvinlin123 alvinlin123 merged commit e34f5e8 into cortexproject:master Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvinlin123 alvinlin123 alvinlin123 approved these changes

@alanprot alanprot alanprot approved these changes

@qinxx108 qinxx108 qinxx108 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@roystchiang @alvinlin123 @alanprot @qinxx108 @rohang98