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

Allow RF greater than number of zones to select more than one instance per zone #5411

Merged

Conversation

justinjung04
Copy link
Contributor

@justinjung04 justinjung04 commented Jun 18, 2023

What this PR does:

ring.Get that should return n (or more) instances which form the replicas for the given key uses the following configs:

  • ring.cfg.ReplicationFactor to control how many replicas you would like to use per key
  • ring.cfg.ZoneAwarenessEnabled to allow replicas to be across the zones evenly

Problem is, when ZoneAwarenessEnabled is enabled and ReplicationFactor is greater than number of available zones, the function was silently ignoring the ReplicationFactor and returned only one instance from each zone:

cortex/pkg/ring/ring.go

Lines 376 to 380 in a307fc0

if r.cfg.ZoneAwarenessEnabled && info.Zone != "" {
if util.StringsContain(distinctZones, info.Zone) {
continue
}
}

RF doesn't need to be bound to number of zones, and the following scenario is valid as well:

  • There are 3 available zones
  • Setting RF to 6 would choose 2 instances from each zone

This PR fixes the function to be able to return max ceil(RF/len(zones)) number of instances per zone. For instance, numOfZones = 3 and RF = 4 will return 4 instances, and each zone will have either 1 or 2 instances returned.

Which issue(s) this PR fixes:
n/a

Checklist

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

…e per zone

Signed-off-by: Justin Jung <jungjust@amazon.com>
@justinjung04 justinjung04 force-pushed the store-gateway-replication-bugfix branch from 19a64f1 to 72f2be0 Compare June 18, 2023 11:22
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jun 18, 2023
Signed-off-by: Justin Jung <jungjust@amazon.com>
@justinjung04 justinjung04 force-pushed the store-gateway-replication-bugfix branch from 34416b1 to 32d36e3 Compare June 18, 2023 11:50
@justinjung04 justinjung04 marked this pull request as ready for review June 18, 2023 12:12
pkg/ring/ring_test.go Outdated Show resolved Hide resolved
Signed-off-by: Justin Jung <jungjust@amazon.com>
Copy link
Contributor

@harry671003 harry671003 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Justin.

Copy link
Collaborator

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good!

@yeya24 yeya24 enabled auto-merge (squash) June 19, 2023 16:19
@yeya24 yeya24 merged commit b98dee6 into cortexproject:master Jun 19, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants