Ring: add method to compute token ranges owned by given instance. #433

pstibrany · 2023-11-17T16:26:37Z

What this PR does:
This PR adds GetTokenRangesForInstance method to *Ring. This method returns token ranges owned by given instance. Implementation is simplified to only support case when zones are enabled, and number of zones is equal to replication factor.

This PR also adds numberOfKeysOwnedByInstance method to the *Ring, which is more universal way of computing key ownership (it supports zones, instance states, and zoneCount!=RF case... but is also 14x slower). This is built on existing code for finding instances for given key, and is used to verify implementation of (*Ring).GetTokenRangesForInstance + (TokenRanges).IncludesKey methods.

Which issue(s) this PR fixes:
We would like to use this code to implement better series limiting in Mimir by considering series ownership.

Checklist

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

duricanikolic · 2023-11-21T07:01:56Z

ring/token_range.go

+
+// TokenRanges describes token ranges owned by an instance.
+// It consists of [start, end] pairs, where both start and end are inclusive.
+type TokenRanges []uint32


I would be more expressive here and I would say something about the length of the slice, and also about the special cases you handle in the implementation.
E.g., this slice contains an even number of elements, elements at even and odd positions represent range starts and ends respectively, etc.

Documenting implementation details would commit to this implementation. Given the early nature of the development, we may still want or need to change it. I prefer to hide implementation details in supporting functions/methods, and only document what's necessary for clients to use.

I agree that we shouldn’t document implementationsl details, but giving an example saying something like “e.g., [5, 10, 20, 30] means that the corresponding instance covers tokens 5-10 and 20-30” wouldn’t damage.

duricanikolic · 2023-11-21T07:05:48Z

ring/token_range.go

+
+// GetTokenRangesForInstance returns the token ranges owned by an instance in the ring.
+//
+// Current implementation only works with multizone setup, where number of zones is equal to replication factor.


I don't know whether it might be of interest for this implementation, but there is a struct called ringToken declared in ownership_priority_queue.go as:

type ringToken struct { token uint32 prevToken uint32 }

I wouldn't reuse the type only because it has same two uint32 fields. I was considering to create custom type with similar fields (start/end), but decided to keep implementation as-is for now, as the code is pretty straightforward to follow (imho).

duricanikolic · 2023-11-21T07:08:49Z

ring/token_range.go

+		return nil, errors.New("no tokens for zone")
+	}
+
+	ranges := make([]uint32, 0, 2*(len(instance.Tokens)+1)) // 1 range (2 values) per token + one additional if we need to split the rollover range


Is the rollover range a range having the start token higher than the end token? E.g., $[2^{32} - 10, 15]$

Is the rollover range a range having the start token higher than the end token? E.g., $[2^{32} - 10, 15]$

Yes, but it's stored as two ranges in the result.

duricanikolic

I left some comments

duricanikolic · 2023-11-21T08:12:37Z

ring/token_range_test.go

+				"instance-0-1": {10, 50, 100},
+			},
+			expected: map[string]TokenRanges{
+				"instance-0-0": {10, 24, 50, 74},


This is something that I personally don’t agree with: if token 10 belongs to an instance, I’d expect the corresponding range to include that token

I agree with you that it's confusing, and wouldn't mind if we fixed that (in some other PR).

I also agree that it's confusing, but it's a function of this code, which causes items that map directly to a token to actually belong to the range owned by the NEXT token.

We could have used half-open ranges in the implementation ([start, end)), but that would have made the IncludesKey logic more complicated, since we'd need to do an additional check if the binarySearch found the key in the ranges slice.

In my opinion, having the token ranges be closed is also easier to reason about, since you don't need to remember the implementation detail of ring instances not actually "owning" any items that map directly to the tokens they have claimed.

Note, by "fixing" this, I didn't mean to change how token ranges work. I agree that closed ranges make sense.

What I meant by fixing is modifying logic in searchToken method, so that if ingester owns the token, it actually "owns" it :)

pracucci

Good job! The logic makes sense to me. I left nits, but I haven't found any issue.

pracucci · 2023-11-21T09:47:44Z

ring/token_range.go

+	if !r.cfg.ZoneAwarenessEnabled || rf != numZones {
+		// if zoneAwareness is disabled we need to treat the whole ring as one big zone, and we would
+		// need to walk the ring backwards looking for RF-1 tokens from other instances to determine the range.
+		return nil, errors.New("can't use ring configuration for computing token ranges")


[nit] I would rather explain "why" telling that this function only support the zone-aware ring with RF = num zones.

I would rather explain "why" telling that this function only support the zone-aware ring with RF = num zones.

True "why" is because we don't need it for our purposes, and it wasn't worth the effort to add support for all cases. I think we should eventually do it, so that Mimir feature using this has full support of various Mimir setups.

I don't think we need to explain this reasoning beyond what's mentioned in the comment:

// Current implementation only works with multizone setup, where number of zones is equal to replication factor.

pracucci · 2023-11-21T10:04:52Z

ring/token_range.go

+	}
+
+	// walk the ring backwards, alternating looking for ends and starts of ranges
+	for i := len(subringTokens) - 1; i > 0; i-- {


[not a blocker] This algorithm complexity is a function of the number of tokens in the ring, so the more instances you have the higher the complexity. The current algorithm as O(N) complexity, where N is the number of tokens.

I think another way to approach this problem is to lookup the instance's tokens and then for each of them finding the range start. The range start can be find using a binary search, so complexity would be O(T * log N), where T is the number of tokens owned by a single instance.

Example:

200 ingesters (per zone)

512 tokens per ingester

Current algorithm complexity: 200 * 512 = 102400

Binary search algorithm complexity: 512 * log(102400) = 512 * 5 = 2560

Thank you. I agree there's room for improvement (also see previous comment about not supporting other cases). @duricanikolic also had some ideas (mentioned privately) how to simplify this.

I will keep the current implementation to focus on the main feature we're working on, but will keep this in mind as possible optimization.

pracucci · 2023-11-21T10:48:19Z

ring/token_range.go

+		} else {
+			// we have a range end, and are looking for the start of the range
+			if info.InstanceID != instanceID {
+				ranges = append(ranges, rangeEnd, token)


I find this hard to read (and the append() below too). I think append(ranges, token, rangeEnd) is easier to read. I understand it doesn't make a difference in the logic cause you sort tokens at the end anyway.

This comment conflicts with the comment below about slices.Reverse().

pracucci · 2023-11-21T10:49:46Z

ring/token_range.go

+
+	// if this instance claimed the first token, it owns the wrap-around range, which we'll break into two separate ranges
+	firstToken := subringTokens[0]
+	firstTokeninfo, ok := r.ringInstanceByToken[firstToken]


[nit]

Suggested change

firstTokeninfo, ok := r.ringInstanceByToken[firstToken]

firstTokenInfo, ok := r.ringInstanceByToken[firstToken]

pracucci · 2023-11-21T10:53:31Z

ring/token_range.go

+	}
+
+	// Ensure returned ranges are sorted.
+	slices.Sort(ranges)


Do we really need to sort it? Isn't a slices.Reverse() what we need?

You're right that reversing should work, if we preserve order of adding of tokens to the slice.

I gave it a quick try, and tests started to fail, so I'm keeping current code to move on. May look at this later.

My plan was to use slices.Reverse() (which is why the appends above are slightly awkward), but I ran into (likely) similar issues to Peter, so I opted to just move on at the time.

pracucci · 2023-11-21T11:00:35Z

ring/token_range_test.go

+	const numZones = 3
+	const numTokens = 512
+	const replicationFactor = numZones // This is the only config supported by GetTokenRangesForInstance right now.
+


[nit] You may want to init and log the rand seed here to be able to eventually reproduce a failure.

I've added initTokenGenerator and passing it to generateRingInstances now, modified many tests in the progress. (But not all, there's still GenerateTokens function that generates random tokens without logging the seed)

Fixed that in other tests too: #437

pracucci · 2023-11-21T11:02:59Z

ring/token_range_test.go

+
+		// find some instance in subring
+		var instanceID string
+		for id := range sr.ringDesc.Ingesters {


[nit] I would run this test for every instance in the ring. It's an easy change to do, but would increase our confidence.

Great idea.

Sure enough, it found bugs!

It turned out that our ring tests can be flaky, because our tests could produce conflicting tokens for different instances. After fixing token generation in tests, test is now reliable.

Stop iteration early in findInstancesForKey, if possible.

… unique tokens.

pstibrany requested a review from pracucci November 17, 2023 16:26

duricanikolic reviewed Nov 21, 2023

View reviewed changes

pracucci approved these changes Nov 21, 2023

View reviewed changes

pstibrany mentioned this pull request Nov 22, 2023

Use seeded random generator in tests. #437

Merged

1 task

pstibrany force-pushed the owned-series-poc branch from 3d4d9da to 82b061d Compare November 22, 2023 08:57

pr00se and others added 21 commits November 22, 2023 13:23

Add function to fetch token ranges for a single instance from the ring

4ebd8ec

Export GetTokenRangesForInstance in ReadRing

71beddc

Error if subringTokens are length 0

d4ac99f

Tiny changes.

8f51c5b

Added method for computing number of tokens owned by instance.

6068ace

Moved token range support into separate file.

dac5392

Stop iteration early in findInstancesForKey, if possible.

Move token range tests.

1490a83

Added comparison benchmark between counting via ring and token ranges.

0b8e321

Introduce TokenRanges type, cleanup tests.

7a0bdf2

Use nil filter, add comment about lock.

9d2efa1

Reverted unnecessary changes.

87ff7e4

Comments.

db6beb5

Make linter happy.

005dcf6

Make linter happy, ep. 2.

6af6b0e

Make go 1.20 happy.

d30058c

Use token generator with logged seed. Not replaced everywhere yet.

6d3fc82

Test all instances.

dbca4e3

log failed instanceID

298b33a

Use same generator.

3ed56e7

Fix last usage of GenerateTokens.

42b5572

Make sure that ring instances generated by generateRingInstances have…

01a1d90

… unique tokens.

pstibrany added 2 commits November 22, 2023 13:23

Comment.

397ecab

Remove unused functions.

958aa2a

pstibrany force-pushed the owned-series-poc branch from 6c87890 to 958aa2a Compare November 22, 2023 12:24

pstibrany added 2 commits November 22, 2023 13:25

empty

00d0718

empty

03251b4

pstibrany merged commit 93246ae into main Nov 22, 2023
3 checks passed

pstibrany deleted the owned-series-poc branch November 22, 2023 14:02

pstibrany mentioned this pull request Nov 22, 2023

Update mimir-prometheus and dskit grafana/mimir#6707

Merged

pstibrany mentioned this pull request Feb 16, 2024

Add GetTokenRangesForPartition method for partition ring #488

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ring: add method to compute token ranges owned by given instance. #433

Ring: add method to compute token ranges owned by given instance. #433

pstibrany commented Nov 17, 2023

duricanikolic Nov 21, 2023

pstibrany Nov 21, 2023

duricanikolic Nov 22, 2023

duricanikolic Nov 21, 2023

pstibrany Nov 21, 2023

duricanikolic Nov 21, 2023

pstibrany Nov 21, 2023 •

edited

duricanikolic left a comment

duricanikolic Nov 21, 2023

pstibrany Nov 21, 2023

pr00se Nov 29, 2023

pstibrany Nov 29, 2023

pracucci left a comment

pracucci Nov 21, 2023

pstibrany Nov 21, 2023

pracucci Nov 21, 2023

pstibrany Nov 21, 2023

pracucci Nov 21, 2023

pracucci Nov 21, 2023

pracucci Nov 21, 2023

pracucci Nov 21, 2023

pstibrany Nov 22, 2023

pstibrany Nov 22, 2023

pr00se Nov 29, 2023

pracucci Nov 21, 2023

pstibrany Nov 21, 2023

pstibrany Nov 22, 2023

pracucci Nov 21, 2023

pstibrany Nov 21, 2023

pstibrany Nov 21, 2023

pstibrany Nov 22, 2023 •

edited

	firstTokeninfo, ok := r.ringInstanceByToken[firstToken]
	firstTokenInfo, ok := r.ringInstanceByToken[firstToken]

Ring: add method to compute token ranges owned by given instance. #433

Ring: add method to compute token ranges owned by given instance. #433

Conversation

pstibrany commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany Nov 21, 2023 • edited

Choose a reason for hiding this comment

duricanikolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany Nov 22, 2023 • edited

Choose a reason for hiding this comment

pstibrany Nov 21, 2023 •

edited

pstibrany Nov 22, 2023 •

edited