abjfh30 commented on PR #29488: URL: https://github.com/apache/flink/pull/29488#issuecomment-6368538482
Thanks both for the review, and for the careful reading of the ordering semantics — much appreciated. I've pushed an update that addresses both comments. Summary of what changed since the last revision: **On the ordering question.** You were right that the javadoc said registration and removal may run concurrently without saying what that ordering means. It now documents the contract: registration and removal are linearized per owner, a resource is released at most once, by the first removal that observes it, and a resource registered after a removal has taken effect starts a new generation that is released only by a later removal of the same owner. So the case you described — a registration landing after the last removal — is a leak rather than a double release. It cannot happen for a given `TieredStoragePartitionId` today, because every registration happens while the partition is created (the tier producer agent constructors and `setupInternal`), and a partition can only be released after `ResultPartition.setup()` has registered it with the `ResultPartitionManager`, which happens after `setupInternal`. I put that obligation in the javadoc rather than only answering here, so it does not have to be re-derived by the next reader. **On `CopyOnWriteArrayList`.** Agreed, and changed to `ArrayList`. Every list mutation now happens inside the map's atomic operations (`compute` / `remove`), so a list is never written after it has been removed from the map, and the iteration in `clearResourceFor` runs after the owner is gone. The map's bin lock provides the happens-before edge, and the release callbacks stay outside the map callback, as you noted. **On the test.** With that mutation discipline in place I could tighten the interleaving case from "released at most once" to "released exactly once": each thread registers a resource and then removes the same owner, so no resource can outlive the loop. Against the old `computeIfAbsent(...).add(...)` shape it now fails with a lost registration (`AtomicInteger(0)` instead of `1`), which the previous assertion accepted. Verified locally with `TieredStorageResourceRegistryTest`, `TieredResultPartitionTest`, `DiskTierProducerAgentTest`, `MemoryTierProducerAgentTest`, `RemoteTierProducerAgentTest` and `RemoteTierMasterAgentTest` (27 tests in total), plus `spotless:check` and `checkstyle:check`. Please take another look when you get a chance. -- 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]
