SEPURI-SAI-KRISHNA commented on PR #29488: URL: https://github.com/apache/flink/pull/29488#issuecomment-6296444458
Non-binding, not a committer. Using `compute` rather than `computeIfAbsent(owner, k -> new ...).add(resource)` is the detail that matters here, since the latter leaves the `add` outside the atomic section. Tests look good, no sleeps. One question. `testConcurrentRegisterAndClearResource` notes that a registration may legally land after the last removal of its owner. In `TieredResultPartition` both calls key on the same `partitionId`, the registration from `setupInternal` and the clear from `releaseInternal`. If a release can overlap that setup, a resource added after `remove` returns sits under a fresh list that nothing clears again, so it is never released. Is that overlap reachable, or does the partition lifecycle rule it out? Worth stating either way in the javadoc you added, which says registration and removal may be concurrent without saying what that order means. Optional: now that every mutation runs inside `compute`, and the removed list cannot be reached once `remove` has returned, `CopyOnWriteArrayList` may be redundant and a plain `ArrayList` would avoid a copy per registration. -- 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]
