tigerquoll commented on PR #1124: URL: https://github.com/apache/yunikorn-core/pull/1124#issuecomment-5466709634
Rebased onto master and pushed. Taking the review points in turn. **go.sum.** Still no change needed: `go mod tidy` on the rebased branch leaves `go.sum` byte-identical to master, because goleak is already pulled in transitively through zap's test dependency. The only conflict in the rebase was `go.mod` (zap/yaml/x-exp bumps on master). **"Fix what we can before turning this on."** Three of the seven have moved since the review: - the test-hygiene half of the stream leak is fixed (YUNIKORN-3372, #1135) - the history-replay send is in review (YUNIKORN-3364, #1133) - the remaining stream case — the in-loop `consumer <- event` that slow-consumer eviction cannot reach, which this PR's table already described as 3(b) — now has its own JIRA, YUNIKORN-3436. It is a production leak (one forwarder plus two 1000-entry buffers pinned per evicted slow client), and `TestEventStreaming_SlowConsumer` is a ready reproduction. It's the same one-line `select` as #1133 applied at the other send. The `ClusterContext` cleaners and `UserGroupCache` (rows 4–6) are the `defer Stop()` fix you asked for. The shim PR (#1061) measured what happens when a test stops the core it started: intermittent failures at `-count>1`, because the services aren't restartable in-process. That's why the test-side `Stop()` work (3366) was folded into YUNIKORN-3370 by its assignee rather than done here — the restart problem has to go first. **Exemptions we are not fixing.** The `notifyRMNewAllocation` entry now says so in the code, per your note that it's benign at process exit. It reproduced in ~43% of `pkg/scheduler/tests` runs when this was opened and 0 of 11 on current master, but the code path is unchanged so the guard stays. **Comments.** All `See YUNIKORN-####` references point at open issues again (3363 and 3366 were closed as duplicates of 3370 after the last push). I also re-probed every exemption on master by deleting it and confirming the check fails without it — all seven are still live, so nothing in the list is stale. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
