query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship #6133

francoposa · 2023-09-25T22:27:54Z

What this PR does

the overall organization of this code is very much open to feedback.
this is supposed to be a pure refactoring PR, not changing behavior

my goals here are to:

pull tenant data out of the userQueue so we can treat it just like a bare queue, in preparation for replacing it with a queue using the multidimensional-fairness-round-robin approach
draw a line between maintaining the the tenant-querier relationships and and doing the actual enqueuing and dequeuing; this new separation is largely demonstrated by the changes to getOrAddQueue (now getOrAddTenantQueue), and getNextQueueForQuerier.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

francoposa · 2023-09-26T17:55:38Z

Okay so everything before 5e47b0b is the super basic refactoring just lifting the fields out of userQueue and up into queues... now to try to encapsulate this a bit

pracucci · 2023-09-26T18:02:30Z

@francoposa For any work on query-scheduler queue, I kindly ask you to get @charleskorn reviews cause he recently worked on it and has a good understanding of it works. Thanks!

francoposa · 2023-09-26T22:18:07Z

pkg/scheduler/queue/user_queues.go

+type userQueue struct {
+	requests *list.List
 }


this could be flattened so it's no longer a separate type at all

When you say flattened, do you mean making this something like type userQueue *list.List? If so, that makes sense to me.

I think I may keep it unflattened for now, for when we move to the newer queue implementation.

That way I can wrap both userQueue and the new queue a a basic queue interface so they can be swapped 1:1.

francoposa · 2023-09-26T22:19:25Z

pkg/scheduler/queue/user_queues.go

-	// We set this to nil if number of available queriers <= maxQueriers.
-	queriers    map[string]struct{}
-	maxQueriers int
+	tenantQuerierState tenantQuerierState


I'm not married to embedding tenantQuerierState into the queues struct, they could be separated.

charleskorn

Overall direction LGTM.

General comments:

We seem to use the terms "user" and "tenant" interchangeably throughout this code, and this was true before this PR as well. It would be good to pick one and use it consistently. (This doesn't have to be done in this PR, this could be a separate PR.)
I'm wondering if we need to rename some types - there are a lot of types with "queue" in their name, which I find confusing. Perhaps we should rename RequestQueue to RequestDispatcher, and queues to tenantQueueManager? Then it's clear that tenantQueueManager is responsible for managing all the different queues for different tenants.

pkg/scheduler/queue/queue.go

charleskorn · 2023-09-27T04:22:04Z

pkg/scheduler/queue/user_queues.go

+	//  - it is detected during request enqueueing that a tenant's queriers
+	//    were calculated from an outdated max-queriers-per-tenant value
+
+	queriersByID map[string]*querierConn


Would be good to clarify which ID we're referring to as the key here.

We have a couple of different IDs stored as strings throughout this file - perhaps adding some specific type definitions would help clarify which one is what and also catch places where we use the wrong kind of ID?

it's a bit verbose because we have to alias search and sort implementations for the slice of querierIDs when we make that a type alias, but I have a good solution - will hold it until your renaming PR gets in because I have to change a bunch of function signatures

Makes sense, thanks.

pkg/scheduler/queue/user_queues.go

charleskorn · 2023-09-27T04:40:22Z

pkg/scheduler/queue/user_queues.go

+type userQueue struct {
+	requests *list.List
 }


When you say flattened, do you mean making this something like type userQueue *list.List? If so, that makes sense to me.

pkg/scheduler/queue/user_queues.go

pkg/scheduler/queue/user_queues_test.go

… named querier

…enantQuerierState

… on tenantQuerierState

…xt tenant for querier

…y tenant ID; push removeTenant down to tenantQuerierAssignments method

… to hide usage of tenantQuerierAssignments from consumers

francoposa · 2023-09-27T23:18:48Z

still WIP on a couple things but stepping away for today

need to re-add that test case for maxQueriers >= available queriers
open question on the mod len(q.users) comment
possible queues to tenantQueueManager rename -> I agree that queues is far too vague, but my first instinct is to avoid naming things somethingManager if possible; would want to brainstorm other possibilities

francoposa · 2023-09-28T17:33:50Z

everything should be addressed for the moment; save for the further merge conflicts that will need corrected once your renaming PR is in.

renamed queues to queueBroker as I believe it best represents the role that struct is playing in mediating the enqueuing and dequeuing.

… search for the slice

… signatures to only take strings rather than QuerierID

francoposa · 2023-09-29T18:35:46Z

@charleskorn I went ahead and made the type alias changes here for querierID and tenantID, as I will be away for a week and a half; feel free to do add whatever you want to carry this refactoring chunk across the line while I am out.

I didn't place a huge emphasis on converting every usage of user to tenant; in part because there's just a lot in this PR already, and in part because I did not want to address yet whether we need to rename files like user_queue too, as some people prefer to avoid file renaming as much as possibly for git history reasons.

charleskorn · 2023-10-02T01:02:23Z

@charleskorn I went ahead and made the type alias changes here for querierID and tenantID, as I will be away for a week and a half; feel free to do add whatever you want to carry this refactoring chunk across the line while I am out.

Awesome, thanks. I'll pick this up once #6141 is merged.

I didn't place a huge emphasis on converting every usage of user to tenant; in part because there's just a lot in this PR already, and in part because I did not want to address yet whether we need to rename files like user_queue too, as some people prefer to avoid file renaming as much as possibly for git history reasons.

Leaving that change to a subsequent PR is fine with me. Renaming the file should be fine, but again, doesn't need to be done as part of this PR.

# Conflicts: # pkg/scheduler/queue/queue.go

charleskorn

LGTM, thanks @francoposa

charleskorn · 2023-10-03T00:55:12Z

@francoposa I'll merge this and leave any follow-up refactoring / cleanup (eg. "user" to "tenant") for you when you're back.

francoposa force-pushed the francoposa/lift-user-data-out-of-userQueue branch from 2cfd90d to 8ded965 Compare September 26, 2023 15:11

pracucci requested a review from charleskorn September 26, 2023 18:01

francoposa changed the title ~~lift user data out of userQueue~~ query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship Sep 26, 2023

francoposa commented Sep 26, 2023

View reviewed changes

charleskorn reviewed Sep 27, 2023

View reviewed changes

francoposa added 18 commits September 27, 2023 13:51

lift user shuffle shard seed out of userQueue

f1d598e

lift user shuffle shard seed out of userQueue

5332749

lift user order index out of userQueue

e19a530

lift user queriers out of userQueue

71b6d12

lift user maxQueriers out of userQueue

0182292

rename type querier -> queueQuerier for now to enable variables to be…

2f82b5c

… named querier

rename type queueQuerier -> querierConn which is a less bad name I think

7a6dfc0

move user id order into tenantQuerierState

9d31b9a

move tenant by ID map into tenantQuerierState

b33ec35

move queriers by ID map into tenantQuerierState

1feb576

move tenant queriers map into tenantQuerierState

7db5580

move tenant querier forget delay into tenantQuerierState

3d259a0

convert tenant querier sharding recompute and shuffle to methods on t…

a0ec248

…enantQuerierState

convert removeQuerier to method on tenantQuerierState

0eb447b

convert removeQuerierConnection to method on tenantQuerierState

2d426f1

convert remaining querier connection management operations to methods…

4a3c737

… on tenantQuerierState

convert getOrAddQueue to use tenantQuerierState to manage tenant

5deea93

convert getNextQueueForQuerier to use tenantQuerierState to report ne…

d753be3

…xt tenant for querier

francoposa force-pushed the francoposa/lift-user-data-out-of-userQueue branch from 153a4c9 to d753be3 Compare September 27, 2023 20:51

francoposa added 3 commits September 27, 2023 15:38

tenantQuerierState -> tenantQuerierAssignments; create const for empt…

67c5afc

…y tenant ID; push removeTenant down to tenantQuerierAssignments method

fix deleteQueue

3232a91

create querier connection management func interfaces on queues object…

88080d7

… to hide usage of tenantQuerierAssignments from consumers

francoposa added 3 commits September 28, 2023 10:12

re-add comment to not use modulo to wrap a list that can change size

ad3f04b

re-add shuffle sharding test case

3c4aef5

rename queues struct to queueBroker

0d576aa

francoposa added 5 commits September 29, 2023 10:19

convert querierID string instances to type alias and implement sort &…

2586d66

… search for the slice

convert querierID string instances to type alias and implement sort &…

cc64106

… search for the slice

Merge branch 'main' into francoposa/lift-user-data-out-of-userQueue

10a2e54

corrections after merge conflict resolution

627a2de

convert tenant ID to type alias and revert RequestQueue public method…

c020d5a

… signatures to only take strings rather than QuerierID

francoposa marked this pull request as ready for review September 29, 2023 18:32

francoposa requested a review from a team as a code owner September 29, 2023 18:32

Merge branch 'main' into francoposa/lift-user-data-out-of-userQueue

82bdbfb

# Conflicts: # pkg/scheduler/queue/queue.go

charleskorn approved these changes Oct 3, 2023

View reviewed changes

charleskorn enabled auto-merge (squash) October 3, 2023 00:54

charleskorn merged commit bd095a2 into main Oct 3, 2023
28 checks passed

charleskorn deleted the francoposa/lift-user-data-out-of-userQueue branch October 3, 2023 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship #6133

query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship #6133

francoposa commented Sep 25, 2023 •

edited

Loading

francoposa commented Sep 26, 2023 •

edited

Loading

pracucci commented Sep 26, 2023

francoposa Sep 26, 2023

charleskorn Sep 27, 2023

francoposa Sep 27, 2023

francoposa Sep 26, 2023

charleskorn left a comment

charleskorn Sep 27, 2023

francoposa Sep 27, 2023

charleskorn Sep 28, 2023

charleskorn Sep 27, 2023

francoposa commented Sep 27, 2023 •

edited

Loading

francoposa commented Sep 28, 2023

francoposa commented Sep 29, 2023 •

edited

Loading

charleskorn commented Oct 2, 2023

charleskorn left a comment

charleskorn commented Oct 3, 2023

query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship #6133

query-scheduler: lift tenant data out of userQueue; encapsulate tenant-querier relationship #6133

Conversation

francoposa commented Sep 25, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

francoposa commented Sep 26, 2023 • edited Loading

pracucci commented Sep 26, 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

charleskorn 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

francoposa commented Sep 27, 2023 • edited Loading

francoposa commented Sep 28, 2023

francoposa commented Sep 29, 2023 • edited Loading

charleskorn commented Oct 2, 2023

charleskorn left a comment

Choose a reason for hiding this comment

charleskorn commented Oct 3, 2023

francoposa commented Sep 25, 2023 •

edited

Loading

francoposa commented Sep 26, 2023 •

edited

Loading

francoposa commented Sep 27, 2023 •

edited

Loading

francoposa commented Sep 29, 2023 •

edited

Loading