Skip to content

Commit 35df1fb

Browse files
rkannan82claude
andcommitted
Rename isSystem to isSystemWorker and add filterWorkers test coverage
- Rename local variable for clarity - Add test for filterWorkers with includeSystemWorkers=false - Annotate bool args with /*includeSystemWorkers*/ for readability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5cce93d commit 35df1fb

2 files changed

Lines changed: 43 additions & 18 deletions

File tree

service/matching/workers/registry_impl.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ type (
3434
hb *workerpb.WorkerHeartbeat
3535
lastSeen time.Time
3636
elem *list.Element
37-
isSystemWorker bool
37+
isSystemWorkerWorker bool
3838
}
3939
// bucket holds part of the keyspace: a map from namespace → (map of instanceKey → entry),
4040
// plus a recency list for eviction.
@@ -107,20 +107,20 @@ func (b *bucket) upsertHeartbeats(nsID namespace.ID, heartbeats []*workerpb.Work
107107
continue
108108
}
109109

110-
isSystem := primitives.IsInternalTaskQueue(hb.GetTaskQueue())
110+
isSystemWorker := primitives.IsInternalTaskQueue(hb.GetTaskQueue())
111111

112112
// Normal upsert
113113
if e, exists := mp[key]; exists {
114114
e.hb = hb
115115
e.lastSeen = now
116-
e.isSystemWorker = isSystem
116+
e.isSystemWorkerWorker = isSystemWorker
117117
b.order.MoveToBack(e.elem)
118118
} else {
119119
e = &entry{
120120
nsID: nsID,
121121
hb: hb,
122122
lastSeen: now,
123-
isSystemWorker: isSystem,
123+
isSystemWorkerWorker: isSystemWorker,
124124
}
125125
e.elem = b.order.PushBack(e)
126126
mp[key] = e
@@ -148,7 +148,7 @@ func (b *bucket) filterWorkers(
148148
}
149149
out := make([]*workerpb.WorkerHeartbeat, 0, len(mp))
150150
for _, e := range mp {
151-
if !includeSystemWorkers && e.isSystemWorker {
151+
if !includeSystemWorkers && e.isSystemWorkerWorker {
152152
continue
153153
}
154154
if predicate(e.hb) {

service/matching/workers/registry_impl_test.go

Lines changed: 38 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"time"
99

1010
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
1112
enumspb "go.temporal.io/api/enums/v1"
1213
workerpb "go.temporal.io/api/worker/v1"
1314
"go.temporal.io/server/common/dynamicconfig"
@@ -40,15 +41,15 @@ func TestUpdateAndListNamespace(t *testing.T) {
4041
defer m.Stop()
4142

4243
// No entries initially
43-
list := m.filterWorkers("ns1", true, alwaysTrue)
44+
list := m.filterWorkers("ns1", true /*includeSystemWorkers*/,alwaysTrue)
4445
assert.Empty(t, list, "expected empty list before updates")
4546

4647
// Add some heartbeats
4748
hb1 := &workerpb.WorkerHeartbeat{WorkerInstanceKey: "workerA", Status: enumspb.WORKER_STATUS_RUNNING}
4849
hb2 := &workerpb.WorkerHeartbeat{WorkerInstanceKey: "workerB", Status: enumspb.WORKER_STATUS_RUNNING}
4950
m.upsertHeartbeats("ns1", []*workerpb.WorkerHeartbeat{hb1, hb2})
5051

51-
list = m.filterWorkers("ns1", true, alwaysTrue)
52+
list = m.filterWorkers("ns1", true /*includeSystemWorkers*/,alwaysTrue)
5253
// Order is not guaranteed; check contents by keys
5354
keys := []string{list[0].WorkerInstanceKey, list[1].WorkerInstanceKey}
5455
assert.Contains(t, keys, "workerA")
@@ -94,7 +95,7 @@ func TestShutdownStatusRemovesWorker(t *testing.T) {
9495
m.upsertHeartbeats("ns1", []*workerpb.WorkerHeartbeat{hb1, hb2})
9596

9697
// Verify both workers are registered
97-
list := m.filterWorkers("ns1", true, alwaysTrue)
98+
list := m.filterWorkers("ns1", true /*includeSystemWorkers*/,alwaysTrue)
9899
assert.Len(t, list, 2, "both workers should be registered")
99100
assert.Equal(t, int64(2), m.total.Load(), "total should be 2")
100101

@@ -103,7 +104,7 @@ func TestShutdownStatusRemovesWorker(t *testing.T) {
103104
m.upsertHeartbeats("ns1", []*workerpb.WorkerHeartbeat{hbShutdown})
104105

105106
// Verify only worker1 is removed, worker2 remains
106-
list = m.filterWorkers("ns1", true, alwaysTrue)
107+
list = m.filterWorkers("ns1", true /*includeSystemWorkers*/,alwaysTrue)
107108
assert.Len(t, list, 1, "only one worker should remain")
108109
assert.Equal(t, "worker2", list[0].WorkerInstanceKey, "worker2 should remain")
109110
assert.Equal(t, int64(1), m.total.Load(), "total should be 1 after shutdown")
@@ -141,7 +142,7 @@ func TestShutdownStatusForNonExistentWorker(t *testing.T) {
141142
m.upsertHeartbeats("ns1", []*workerpb.WorkerHeartbeat{hb})
142143

143144
// Verify nothing happened
144-
list := m.filterWorkers("ns1", true, alwaysTrue)
145+
list := m.filterWorkers("ns1", true /*includeSystemWorkers*/,alwaysTrue)
145146
assert.Empty(t, list, "no workers should exist")
146147
assert.Zero(t, m.total.Load(), "total should remain 0")
147148
}
@@ -180,12 +181,36 @@ func TestListNamespacePredicate(t *testing.T) {
180181

181182
for _, tc := range tests {
182183
t.Run(tc.name, func(t *testing.T) {
183-
list := m.filterWorkers("ns", true, tc.pred)
184+
list := m.filterWorkers("ns", true /*includeSystemWorkers*/,tc.pred)
184185
assert.Len(t, list, tc.wantCount)
185186
})
186187
}
187188
}
188189

190+
func TestFilterWorkersExcludesSystemWorkers(t *testing.T) {
191+
m := newRegistryImpl(testDefaultRegistryParams(metrics.NoopMetricsHandler))
192+
defer m.Stop()
193+
194+
m.upsertHeartbeats("ns", []*workerpb.WorkerHeartbeat{
195+
{WorkerInstanceKey: "user-1", TaskQueue: "my-queue"},
196+
{WorkerInstanceKey: "user-2", TaskQueue: "my-queue"},
197+
{WorkerInstanceKey: "sys-1", TaskQueue: "temporal-sys-per-ns-tq"},
198+
})
199+
200+
t.Run("includeSystemWorkers=true returns all", func(t *testing.T) {
201+
list := m.filterWorkers("ns", true /*includeSystemWorkers*/, alwaysTrue)
202+
require.Len(t, list, 3)
203+
})
204+
205+
t.Run("includeSystemWorkers=false excludes system workers", func(t *testing.T) {
206+
list := m.filterWorkers("ns", false /*includeSystemWorkers*/, alwaysTrue)
207+
require.Len(t, list, 2)
208+
for _, w := range list {
209+
require.NotEqual(t, "sys-1", w.WorkerInstanceKey)
210+
}
211+
})
212+
}
213+
189214
func TestEvictByTTL(t *testing.T) {
190215
// Use capture handler to verify metrics
191216
captureHandler := metricstest.NewCaptureHandler()
@@ -216,7 +241,7 @@ func TestEvictByTTL(t *testing.T) {
216241
// Perform eviction
217242
m.evictByTTL()
218243

219-
list := m.filterWorkers("ns", true, alwaysTrue)
244+
list := m.filterWorkers("ns", true /*includeSystemWorkers*/,alwaysTrue)
220245
assert.Empty(t, list, "entry should be evicted by TTL")
221246
assert.Zero(t, m.total.Load(), "total counter should be decremented")
222247

@@ -261,7 +286,7 @@ func TestEvictByCapacity(t *testing.T) {
261286
m.evictByCapacity()
262287

263288
// Ensure we evicted down to maxItems
264-
remaining := m.filterWorkers("ns", true, alwaysTrue)
289+
remaining := m.filterWorkers("ns", true /*includeSystemWorkers*/,alwaysTrue)
265290
assert.Len(t, remaining, int(maxItems), "should evict down to maxItems")
266291
assert.LessOrEqual(t, m.total.Load(), int64(maxItems), "total counter should not exceed maxItems")
267292

@@ -323,7 +348,7 @@ func TestEvictByCapacityWithMinAgeProtection(t *testing.T) {
323348
m.evictByCapacity()
324349

325350
// All entries should still be there (protected by minEvictAge)
326-
workers := m.filterWorkers("ns", true, alwaysTrue)
351+
workers := m.filterWorkers("ns", true /*includeSystemWorkers*/,alwaysTrue)
327352
assert.Len(t, workers, 3, "all entries should be protected by minEvictAge")
328353
assert.Equal(t, int64(3), m.total.Load(), "should still exceed maxItems due to protection")
329354

@@ -374,7 +399,7 @@ func TestEvictByCapacityAfterMinAge(t *testing.T) {
374399
m.evictByCapacity()
375400

376401
// Should have evicted down to maxItems
377-
workers := m.filterWorkers("ns", true, alwaysTrue)
402+
workers := m.filterWorkers("ns", true /*includeSystemWorkers*/,alwaysTrue)
378403
assert.LessOrEqual(t, len(workers), int(maxItems), "should evict down to maxItems")
379404
assert.LessOrEqual(t, m.total.Load(), int64(maxItems), "total should be within limits")
380405

@@ -424,10 +449,10 @@ func TestMultipleNamespaces(t *testing.T) {
424449
m.upsertHeartbeats("namespace2", ns2Workers)
425450

426451
// Verify functional behavior first
427-
ns1List := m.filterWorkers("namespace1", true, alwaysTrue)
452+
ns1List := m.filterWorkers("namespace1", true /*includeSystemWorkers*/,alwaysTrue)
428453
assert.Len(t, ns1List, 3, "namespace1 should have 3 workers")
429454

430-
ns2List := m.filterWorkers("namespace2", true, alwaysTrue)
455+
ns2List := m.filterWorkers("namespace2", true /*includeSystemWorkers*/,alwaysTrue)
431456
assert.Len(t, ns2List, 2, "namespace2 should have 2 workers")
432457

433458
assert.Equal(t, int64(5), m.total.Load(), "total should be 5 workers across namespaces")
@@ -548,7 +573,7 @@ func BenchmarkListNamespace(b *testing.B) {
548573
}
549574
b.ResetTimer()
550575
for i := 0; i < b.N; i++ {
551-
_ = m.filterWorkers("benchNs", true, alwaysTrue)
576+
_ = m.filterWorkers("benchNs", true /*includeSystemWorkers*/,alwaysTrue)
552577
}
553578
}
554579

0 commit comments

Comments
 (0)