feat(ring): support for owning multiple partitions #725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
colega
changed the title
colega
force-pushed
the
feat-ring-support-for-owning-multiple-partitions
branch
from
578c87f to
f922d4b
Compare
Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Oleg Zaytsev <[email protected]>
colega
force-pushed
the
feat-ring-support-for-owning-multiple-partitions
branch
from
f922d4b to
e312311
Compare
pracucci
approved these changes
Jul 15, 2025
Contributor
pracucci
left a comment
pracucci
left a comment
There was a problem hiding this comment.
Good job! I only left nits.
| func (r *MultiPartitionInstanceRing) PartitionRing() *PartitionRing { | ||
| return r.partitionsRingReader.PartitionRing() | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
[nit] I suggest to add InstanceRing() like PartitionInstanceRing. It's legit to get the instances ring from MultiPartitionInstanceRing.
Suggested change
| func (r * MultiPartitionInstanceRing) InstanceRing() InstanceRingReader { | |
| return r.instancesRing | |
| } | |
Contributor
Author
There was a problem hiding this comment.
I explicitly left it unimplemented because we don't use it in the implementation, and if I add it, I'd feel to be forced to use it, but I think that could make unit-testing harder.
I'd rather leave it like this for now.
|
|
||
| // this method expects instanceIDs to be in the same order as instances. | ||
| // instanceIDs should hold the parsed multi-partition owner IDs. | ||
| func highestPreferrablyNonReadOnlyFromEachZone(instances []InstanceDesc, instanceIDs []string, instanceZones []string) []InstanceDesc { |
Contributor
There was a problem hiding this comment.
[nit] Typo
Suggested change
| func highestPreferrablyNonReadOnlyFromEachZone(instances []InstanceDesc, instanceIDs []string, instanceZones []string) []InstanceDesc { | |
| func highestPreferablyNonReadOnlyFromEachZone(instances []InstanceDesc, instanceIDs []string, instanceZones []string) []InstanceDesc { |
| }, nil | ||
| } | ||
|
|
||
| // this method expects instanceIDs to be in the same order as instances. |
Contributor
There was a problem hiding this comment.
[nit] Please also comment that instances is updated in-place and then returned.
ring/partition_ring.go
Outdated
| } | ||
|
|
||
| for i, ownerID := range ids { | ||
| if p := strings.IndexByte(ownerID, '/'); p != -1 { |
Contributor
There was a problem hiding this comment.
This search for the 1st / occurrence, but so far I don't a reason why / couldn't also be part of the instance ID. I think using strings.LastIndexByte() would more correct.
Contributor
Author
There was a problem hiding this comment.
Right, that's also more efficient. Fixed and added a test with an instance ID with a slash. 1c09d6b
Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Oleg Zaytsev <[email protected]>
Merged
1 task
salvacorts
added a commit
that referenced
this pull request
Aug 4, 2025
… arch compatibility (#730) Loki is failing to build for the linux/arm platform with ([see][1]): ``` 59.88 # github.com/grafana/dskit/ring 59.88 vendor/github.com/grafana/dskit/ring/multi_partition_instance_ring.go:151:10: cannot use math.MaxInt64 (untyped int constant 9223372036854775807) as int value in return statement (overflows) 59.88 vendor/github.com/grafana/dskit/ring/multi_partition_instance_ring.go:155:10: cannot use math.MaxInt64 (untyped int constant 9223372036854775807) as int value in return statement (overflows) ``` linux/arm is a 32bits platform so int is 32 bits, but `indexFromInstanceSuffix` returns `math.MaxInt64` instead of `math.MaxInt`. **What this PR does**: **Which issue(s) this PR fixes**: Fixes PR - #725 **Checklist** - [ ] Tests updated [1]: https://github.com/grafana/loki/actions/runs/16723281173/job/47333518579#step:9:486
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
This adds a new
MultiPartitionInstanceRingas an alternative toPartitionInstanceRingin which the instances can own more than one partition. It uses the same instance PartitionRing desc, so we leverage that by setting a different instance ID as owner of each partition.The convention is enclosed in this implementation and consists of adding a suffix
/<partition>to the instance ID when it ownspartition. We initially added this as an option toPartitionInstanceRing, however most of the code actually differs, and the fact that you could instantiate aPartitionInstanceRingand use it in a multi-partition context introduced subtle bugs in the code. By adding a specific type to this we enforce that each implementation will consistently use one or another.We do add this as an option to
PartitionInstanceLifecycler, because there most of the code is shared.PartitionRingexposes a new methodMultiPartitionOwnerIDsthat will list the real (non-suffixed) instance IDs of partition owners ready to be used by any code.Finally, we also add a method to
PartitionRingEditorto remove an instance ID as an owner of a partition, required to perform the periodic cleanup of the stale instances in the ring by users of this library.Checklist