fix issue where we are not returning all the available ingesters in the ring #3977
Conversation
|
With this change, we should see less CPU usage during deployment too, as you don't have to loop as many times when 1 ingester went unhealthy when |
roystchiang
force-pushed
the
fix-ring-issue
branch
2 times, most recently
from
70294d9 to
843d4f4
Compare
…he ring (cortexproject#3977) Signed-off-by: Roy Chiang <[email protected]>
roystchiang
force-pushed
the
fix-ring-issue
branch
from
843d4f4 to
eeabb3a
Compare
pkg/ring/ring.go
Outdated
| // this instance. | ||
| shouldExtendReplicaSet := op.ShouldExtendReplicaSetOnState(instance.State) | ||
| if shouldExtendReplicaSet { | ||
| n++ |
There was a problem hiding this comment.
I don't think it should be extended here. Reason is that at this point you still don't know if the instance belongs to a zone which has already been included and so the following check on distinctZones will discard it.
Keeping the logic in master, I a fix could be just moving distinctZones = append(distinctZones, info.Zone) to an else branch of the check on ShouldExtendReplicaSetOnState():
// Check whether the replica set should be extended given we're including
// this instance.
if op.ShouldExtendReplicaSetOnState(instance.State) {
n++
} else if r.cfg.ZoneAwarenessEnabled && info.Zone != "" {
// TODO add a comment here to explain the rationale
distinctZones = append(distinctZones, info.Zone)
}
pkg/ring/ring_test.go
Outdated
| "instance-1": {Addr: "127.0.0.1", Zone: "zone-a", Tokens: GenerateTokens(128, nil), State: ACTIVE}, | ||
| "instance-2": {Addr: "127.0.0.2", Zone: "zone-a", Tokens: GenerateTokens(128, nil), State: ACTIVE}, | ||
| "instance-3": {Addr: "127.0.0.3", Zone: "zone-b", Tokens: GenerateTokens(128, nil), State: ACTIVE}, | ||
| "instance-4": {Addr: "127.0.0.4", Zone: "zone-b", Tokens: GenerateTokens(128, nil), State: ACTIVE}, | ||
| "instance-5": {Addr: "127.0.0.5", Zone: "zone-c", Tokens: GenerateTokens(128, nil), State: LEAVING}, | ||
| "instance-6": {Addr: "127.0.0.6", Zone: "zone-c", Tokens: GenerateTokens(128, nil), State: ACTIVE}, |
There was a problem hiding this comment.
Here you're generating Tokens() but then they're overwritten below in the for loop. You can remove them from here.
pkg/ring/ring_test.go
Outdated
| assert.Equal(t, len(set.Instances), 3) | ||
| assert.Equal(t, len(distinctZones), 3) |
There was a problem hiding this comment.
[nit] Ordering of arguments is (t, expected, actual), so they should be flipped. However, to check length you can use assert.Len().
roystchiang
force-pushed
the
fix-ring-issue
branch
from
fe7e913 to
94f0463
Compare
Signed-off-by: Roy Chiang <[email protected]>
roystchiang
force-pushed
the
fix-ring-issue
branch
from
94f0463 to
9f5aa39
Compare
|
@roystchiang Please make sure to sign your commits (see here) otherwise we can't merge. |
Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Roy Chiang <[email protected]>
roystchiang
force-pushed
the
fix-ring-issue
branch
from
9096725 to
fe7607b
Compare
oops. Sorry. I will sign. |
What this PR does:
Fix an issue where extended write does not return all the available instances.
If we checked ingester-1 that's in
az-A, and it's not active. We'll addaz-Ato distinct zones in the subsequent loops, we'll never check ingesters inaz-Aagain.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]