fix: only re-prioritize the compaction queue when the config changes

Project: milvus-io/milvus | Language: Go | Concepts: concurrency, data-structures, error-handling | Difficulty: mid
Link: https://github.com/milvus-io/milvus/pull/52260

The problem

The compaction scheduler in Milvus was incorrectly re-prioritizing the entire compaction queue on every scheduling tick (every 500ms), regardless of whether the prioritization configuration had changed. This was due to an invalid comparison of function pointers that always evaluated to true. Additionally, there was a data race in updating the prioritizer: the prioritizer function was being assigned outside the protection of a lock, while other methods accessed it under lock.

Why it's non-trivial

A naive fix might try to compare function values directly with ==, which is not allowed in Go for function types. Alternatively, a fix that only locks the assignment but not the comparison could still leave subtle concurrency bugs. The solution must ensure:

• The queue is only re-prioritized when the prioritizer configuration actually changes.
• All prioritizer updates and accesses are thread-safe.
• The logic distinguishes between "no configuration set" and "empty string" as a configuration value, since both are possible and meaningful.

The approach

1. Remove the broken function pointer comparison

Previously, the scheduler attempted to check if the prioritizer had changed using an address comparison that always failed:

-p := getPrioritizer()
-if &c.queueTasks.prioritizer != &p {   // address of a field vs address of a local
-    c.queueTasks.UpdatePrioritizer(p)  // -> always taken
-}

What this did:
This code always re-prioritized the queue on every tick because the addresses of two different function variables are never equal in Go, even if they point to the same function.

2. Track prioritizer identity by name, not by function value

The compaction queue now stores both the prioritizer function and its configuration name, both protected by a lock:

-type CompactionQueue struct {
-    pq          PriorityQueue[CompactionTask]
-    lock        lock.RWMutex
-    prioritizer Prioritizer
-    capacity    int
+type CompactionQueue struct {
+    pq   PriorityQueue[CompactionTask]
+    lock lock.RWMutex
+    // prioritizer and prioritizerName are guarded by lock.
+    // Prioritizer is a func value and therefore cannot be compared with ==,
+    // so the configuration name is kept alongside it as its identity.
+    //
+    // prioritizerName is nil while prioritizer has not been resolved from the
+    // configuration -- the constructor and UpdatePrioritizer both take a func
+    // directly. It is a pointer rather than a string because "" is itself a
+    // settable configuration value (the RESTful alterConfig contract treats a
+    // present value, including the empty string, as a set and only null as a
+    // reset, and dataCoord.compaction.taskPrioritizer has no Formatter), so a
+    // string zero value cannot stand in for "unset" without colliding with it.
+    prioritizer     Prioritizer
+    prioritizerName *string
+    capacity        int
}

Why:
Function values cannot be compared directly in Go. By tracking the configuration name, the code can safely determine if the prioritizer has changed.

3. Add atomic, lock-guarded prioritizer sync

A new method, SyncPrioritizer, compares the current prioritizer name with the new one under a lock, and only updates (and re-prioritizes the queue) if the name actually changed:

+// SyncPrioritizer re-prioritizes the queue only when the configured prioritizer
+// actually changed. It is safe to call on every scheduling tick.
+func (q *CompactionQueue) SyncPrioritizer(name string) {
+    q.lock.Lock()
+    defer q.lock.Unlock()
+    if q.prioritizerName != nil && *q.prioritizerName == name {
+        return
+    }
+    q.prioritizerName = &name
+    q.updatePrioritizerLocked(getPrioritizerByName(name))
+}

Why:
This ensures the queue is only re-prioritized when necessary, and all state is updated atomically under the lock.

4. Fix the data race in UpdatePrioritizer

The assignment to the prioritizer function is now done under the lock, not before it:

-func (q *CompactionQueue) UpdatePrioritizer(prioritizer Prioritizer) {
-    q.prioritizer = prioritizer
-    q.lock.Lock()
-    defer q.lock.Unlock()
+func (q *CompactionQueue) UpdatePrioritizer(prioritizer Prioritizer) {
+    q.lock.Lock()
+    defer q.lock.Unlock()
+    q.prioritizerName = nil
+    q.updatePrioritizerLocked(prioritizer)
}

Why:
This prevents concurrent reads/writes to the prioritizer function, eliminating the data race.

5. Split prioritizer resolution into name and function

Instead of a single function that returns a prioritizer, the code now has:

+func getPrioritizerName() string {
+    return Params.DataCoordCfg.CompactionTaskPrioritizer.GetValue()
+}
+
+func getPrioritizerByName(name string) Prioritizer {
+    switch name {
+    case "level":
+        return LevelPrioritizer
+    case "mix":
+        return MixFirstPrioritizer
+    default:
+        return DefaultPrioritizer
+    }
+}

Why:
This keeps the name and function value in sync, and allows for correct identity checks.

6. Comprehensive tests

A new test, TestCompactionQueue_SyncPrioritizer, checks:

• No re-prioritization occurs if the name doesn't change.
• Empty string as a configuration value is handled correctly.
• Out-of-band updates clear the name so the next sync works.
• Data race is absent under concurrent access.

From the review thread

No notable review discussion on this PR.

Takeaways

• Do not compare function values for identity in Go; use a separate identifier (like a string name) if you need to track configuration changes.
• Always perform concurrent state updates (especially for shared resources) under appropriate locks to avoid data races.
• When a configuration can be "unset" or explicitly set to an empty value, model this distinction explicitly (e.g., use a pointer or sentinel value), rather than relying on the zero value of a type.