-
Notifications
You must be signed in to change notification settings - Fork 68
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
Lifecycler: return the number of instances in lifecycler's zone #266
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.
LGTM (modulo a couple of nits). Thanks!
25f10e7
to
b392a57
Compare
if _, ok := zones[ingester.Zone]; !ok { | ||
zones[ingester.Zone] = 0 | ||
} |
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.
Nit: Setting default value 0
to zones[ingester.Zone]
is unnecessary. zones[ingester.Zone]++
below will work just fine without this.
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.
Hi @pstibrany,
This assignment is needed in order to count the total number of zones. If we remove the block setting 0
to zones[ingester.Zone]
, we will count only the number of zones with healthy ingesters.
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.
Good point that I missed! That's worth adding a comment in my opinion.
What this PR does:
This PR enriches
Lifecycler
with theInstancesInZoneCount()
method, that returns the number of instances in ring belonging to the lifecycler's zone, updated during the last heartbeat period.A test (
TestLifecycler_InstancesInZoneCount
) showing the expected behaviour ofInstancesInZoneCount()
has been added.Which issue(s) this PR fixes:
This PR is a pre-requisite for fixing the issue 4208.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]