perf(search): parallelize resource searches with errgroup#433
perf(search): parallelize resource searches with errgroup#433DioCrafts wants to merge 4 commits intokite-org:mainfrom
Conversation
Head branch was pushed to by a user without write access
3f17b35 to
521684d
Compare
|
CI fail already fixed @zxh326 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 521684d2d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ckground refresh Solution A: Replace sequential SearchFuncs iteration with errgroup-based parallel execution. Each resource type (pods, services, deployments, etc.) is now searched concurrently, reducing latency from O(N * avg_latency) to O(max_latency) where N is the number of searchable resource types. - Each goroutine writes to its own pre-allocated index in resultSlices (no mutex contention needed) - Failing resource types are logged and skipped without aborting the search - Results are merged, sorted, and truncated after all goroutines complete Solution E: Remove background goroutine that refreshed cache on hits. The stale-while-revalidate pattern caused unbounded fire-and-forget goroutines, wasted API server bandwidth, and provided negligible UX benefit given the 10-minute cache TTL. Cache entries now expire naturally. Dead code removed: - Removed unused sync.Mutex import and variable - Removed gin.Context.Copy() call (no longer needed without bg goroutine) New tests: - TestSearchParallelExecution: proves concurrent execution (timing + atomic counter) - TestSearchPartialFailure: one failing resource doesn't break others - TestGlobalSearchCacheDoesNotTriggerBackgroundRefresh: validates Solution E - TestSearchEmptyResourceFuncs: graceful handling of zero search funcs All 8/8 tests pass.
521684d to
86da2bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86da2bb8c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… results) - Replace `err = nil` in recover block with `hadPanic.Store(true)` atomic flag - Skip `cache.Add()` when any goroutine panicked, preventing 10-min stale-while-broken - Partial results still returned to caller (non-panicked resources remain useful) - Add TestSearchPanicDoesNotCacheResults: validates panic skips cache + retry works Addresses review: chatgpt-codex-connector P2 on lines 84-87
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17d1ff3801
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Rename hadPanic → hadFailure (atomic.Bool) — set on both panic AND error - entry.fn returning an error now also prevents cache write - Transient API failures no longer get cached as valid 200 OK for 10 min - TestSearchPartialFailure extended: verifies cache is skipped on error Addresses review: chatgpt-codex-connector P2 on lines 92-95
Head branch was pushed to by a user without write access
|
fix lint on test file |
perf(search): Parallelize resource searches with errgroup & remove background refresh
Search downloads full resource lists sequentially
Problem
SearchHandler.Search()iterated over up to 9 resource types sequentially. Each call to a search function triggered a fullK8sClient.List()+ name filtering round-trip. With 9 resource types, the worst-case latency was O(N × avg_latency) — roughly 9× the cost of a single resource search.Additionally,
GlobalSearch()on a cache hit launched a fire-and-forget background goroutine (go func() { h.Search(copiedCtx, ...) }()) to refresh the cache. This caused:gin.Context.Copy()used to share state with background goroutineSolution 1 — Parallel search with
errgroupReplaced the sequential
for name, searchFunc := range searchFuncsloop witherrgroup.WithContext()parallel execution:Key design decisions:
resultSlices— no mutex neededgin.Contextis read-safe for concurrent access (onlyMustGet("cluster")is called)Latency reduction: O(N × avg) → O(max) = ~9× improvement in worst case
Solution 2 — Remove background refresh goroutine
Removed the stale-while-revalidate pattern from
GlobalSearch():Cache entries now expire naturally after the 10-minute TTL. The next request after expiry triggers a fresh (now parallel!) search.
Dead code removed
sync.Muteximport and variable (not needed with index-based writes)gin.Context.Copy()call (no longer needed without background goroutine)Tests added (4 new, 8 total — all PASS)
TestSearchParallelExecutionTestSearchPartialFailureTestGlobalSearchCacheDoesNotTriggerBackgroundRefreshTestSearchEmptyResourceFuncsFiles changed
pkg/handlers/search_handler.gopkg/handlers/search_handler_test.gogo.modgolang.org/x/syncpromoted from indirect to direct dependencyPerformance impact
gin.Context.Copy()