-
Notifications
You must be signed in to change notification settings - Fork 802
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
Using subring on getShardedRules when shuffle sharding #4466
Using subring on getShardedRules when shuffle sharding #4466
Conversation
b7a0438
to
4b4204e
Compare
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.
Thank you, LGTM.
1ed3818
to
d3681b6
Compare
Signed-off-by: Alan Protasio <approtas@amazon.com>
Signed-off-by: Alan Protasio <approtas@amazon.com>
d3681b6
to
25280c8
Compare
Hi... Can i get some attention on this PR? Situation like #4459 makes 100% of the APIs fail right now. |
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
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.
Makes sense to me, thanks!
…#4466) * Using subring on getShardedRules when shuffle sharding Signed-off-by: Alan Protasio <approtas@amazon.com> Signed-off-by: Manish Kumar Gupta <manishkg@microsoft.com>
…#4466) * Using subring on getShardedRules when shuffle sharding Signed-off-by: Alan Protasio <approtas@amazon.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
What this PR does:
GetRules API should call only the rulers on the tenant's subring when sharding strategy is set to
ShardingStrategyShuffle
.Right now we are calling all rulers in the
GetRules
api even if the sharding strategy is set toShardingStrategyShuffle
. In this case, we should only call the rules in the tenant subring as those rules are the only ones that can load rules from this tenant. This change is specially important as right now we need all the rulers in the ring to be available or cortex will return an error - this means that if we have shardSize = 3 but 100 rulers in total, we will return 5xx if any of the 100 rules are down (for instance during deployment), even though the callers rules are spread only on 3 of them.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]