Use node fields instead of custom queuing algorithm to count children…

… 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

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{}
 

pkg/scheduler/queue/tenant_querier_assignment.go

@@ -451,7 +451,7 @@ func (tqa *tenantQuerierAssignments) dequeueSelectNode(node *Node) (*Node, bool)
 		return nil, true
 	}
 
-	checkedAllNodes := node.childrenChecked == len(node.queueMap)+1 // must check local queue as well
+	checkedAllNodes := node.childrenChecked == len(node.queueMap)
 
 	// advance queue position for dequeue
 	tqa.tenantOrderIndex++
@@ -484,8 +484,6 @@ func (tqa *tenantQuerierAssignments) dequeueSelectNode(node *Node) (*Node, bool)
 			continue
 		}
 
-		checkedAllNodes = node.childrenChecked == len(node.queueMap)+1
-
 		// if the tenant-querier set is nil, any querier can serve this tenant
 		if tqa.tenantQuerierIDs[tenantID] == nil {
 			tqa.tenantOrderIndex = checkIndex

pkg/scheduler/queue/tree_queue_algo_querier_worker_queue_priority.go

@@ -59,7 +59,6 @@ type QuerierWorkerQueuePriorityAlgo struct {
 	currentNodeOrderIndex int
 	nodeOrder             []string
 	nodeCounts            map[string]int
-	nodesChecked          int
 }
 
 func NewQuerierWorkerQueuePriorityAlgo() *QuerierWorkerQueuePriorityAlgo {
@@ -87,8 +86,8 @@ func (qa *QuerierWorkerQueuePriorityAlgo) wrapCurrentNodeOrderIndex(increment bo
 	}
 }
 
-func (qa *QuerierWorkerQueuePriorityAlgo) checkedAllNodes() bool {
-	return qa.nodesChecked == len(qa.nodeOrder)
+func (qa *QuerierWorkerQueuePriorityAlgo) checkedAllNodes(n *Node) bool {
+	return n.childrenChecked == len(n.queueMap)
 }
 
 func (qa *QuerierWorkerQueuePriorityAlgo) addChildNode(parent, child *Node) {
@@ -118,11 +117,11 @@ func (qa *QuerierWorkerQueuePriorityAlgo) addChildNode(parent, child *Node) {
 
 func (qa *QuerierWorkerQueuePriorityAlgo) dequeueSelectNode(node *Node) (*Node, bool) {
 	currentNodeName := qa.nodeOrder[qa.currentNodeOrderIndex]
-	if node, ok := node.queueMap[currentNodeName]; ok {
-		qa.nodesChecked++
-		return node, qa.checkedAllNodes()
+	checkedAllNodes := qa.checkedAllNodes(node)
+	if childNode, ok := node.queueMap[currentNodeName]; ok {
+		return childNode, checkedAllNodes
 	}
-	return nil, qa.checkedAllNodes()
+	return nil, checkedAllNodes
 }
 
 func (qa *QuerierWorkerQueuePriorityAlgo) dequeueUpdateState(node *Node, dequeuedFrom *Node) {
@@ -155,7 +154,4 @@ func (qa *QuerierWorkerQueuePriorityAlgo) dequeueUpdateState(node *Node, dequeue
 		// delete child node from its parent's queueMap
 		delete(node.queueMap, childName)
 	}
-
-	// reset state after successful dequeue
-	qa.nodesChecked = 0
 }