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

Use node fields instead of custom queuing algorithm to count children checked #9154

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

chencs
Copy link
Contributor

@chencs chencs commented Aug 30, 2024

What this PR does

Updates QuerierWorkerQueuePriorityAlgo to use the Node's childrenChecked field instead of keeping its own count of nodes checked; this lets the tree reset the count after the dequeue operation is completed. This also includes a small bugfix introduced to tenantQuerierAssignments by defining leaf nodes.

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].
  • about-versioning.md updated with experimental features.

@chencs chencs requested a review from a team as a code owner August 30, 2024 22:47
@chencs chencs force-pushed the casie/use-node-field-instead-of-qa branch from 1b23c2e to 8ff7631 Compare August 30, 2024 23:19
Copy link
Member

@francoposa francoposa left a comment

Choose a reason for hiding this comment

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

Approved, pending the below patch to the benchmark test which caught some bugs when I was exploring an alternate implementation.
(artistic license allowed on the comment wording)

% git diff pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_benchmark_test.go
diff --git a/pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_benchmark_test.go b/pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_benchmark_test.go
index 95e7ea8a3..34e455d8d 100644
--- a/pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_benchmark_test.go
+++ b/pkg/scheduler/queue/multi_queuing_algorithm_tree_queue_benchmark_test.go
@@ -347,8 +347,6 @@ func TestMultiDimensionalQueueAlgorithmSlowConsumerEffects(t *testing.T) {
                },
        }
 
-       maxQueriersPerTenant := 0 // disable shuffle sharding
-
        // Increase totalRequests to tighten up variations when running locally, but do not commit higher values;
        // the later test cases with a higher percentage of slow queries will take a long time to run.
        totalRequests := 1000
@@ -365,6 +363,12 @@ func TestMultiDimensionalQueueAlgorithmSlowConsumerEffects(t *testing.T) {
        // an ingester can respond in 0.3 seconds while a slow store-gateway query can take 30 seconds
        slowConsumerLatency := 100 * time.Millisecond
 
+       // enable shuffle sharding; we cannot be too restrictive with only two tenants,
+       // or some consumers will not get sharded to any of the two tenants.
+       // enabling shuffle sharding ensures we will hit cases where a querier-worker
+       // does not find any tenant leaf nodes it can work on under its prioritized query component node
+       maxQueriersPerTenant := numConsumers - 1
+
        var testCaseNames []string
        testCaseReports := map[string]*testScenarioQueueDurationReport{}

@chencs chencs merged commit 0084380 into main Sep 2, 2024
29 checks passed
@chencs chencs deleted the casie/use-node-field-instead-of-qa branch September 2, 2024 22:53
grafanabot pushed a commit that referenced this pull request Sep 3, 2024
… checked (#9154)

* Use node fields instead of custom queuing algorithm to count children checked

* Fix subtle bug introduced by defining leaf nodes

* add Franco's patch to benchmark test

(cherry picked from commit 0084380)
francoposa pushed a commit that referenced this pull request Sep 3, 2024
… checked (#9154) (#9187)

* Use node fields instead of custom queuing algorithm to count children checked

* Fix subtle bug introduced by defining leaf nodes

* add Franco's patch to benchmark test

(cherry picked from commit 0084380)

Co-authored-by: chencs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants