kerwin-zk commented on PR #13283: URL: https://github.com/apache/gluten/pull/13283#issuecomment-6219007388
[Blocking] The new regression test still does not cover the manager-ownership bug fixed by this PR. [`CelebornUtilsTest`](https://github.com/apache/gluten/blob/7ac2456834d15cfa7897273104fa3038ebdc9024/gluten-celeborn/src/test/java/org/apache/spark/shuffle/gluten/celeborn/CelebornUtilsTest.java#L135-L172) calls `fetchCleanFailedShuffle` directly and then calls `stopFailedShuffleCleaner` directly; it never exercises `CelebornShuffleManager`. The callback verifications only prove that `fetchCleanFailedShuffle` registered callbacks, not that [`initializeLifecycleManager()` stores the returned cleaner](https://github.com/apache/gluten/blob/7ac2456834d15cfa7897273104fa3038ebdc9024/gluten-celeborn/src/main/java/org/apache/spark/shuffle/gluten/celeborn/CelebornShuffleManager.java#L182-L183) or that [`CelebornShuffleManager.stop()` releases it](https://github.com/apache/gluten/blob/7ac2456834d15cfa7897273104fa3038ebdc9024/gluten-celeborn/src/main/java/org/apache/spark/shuffle/gluten/celebo rn/CelebornShuffleManager.java#L283-L285). Reverting either manager path would leave all of these assertions green. Please add a manager-level regression test with failed-shuffle cleanup enabled that verifies the manager retains the cleaner and that calling `CelebornShuffleManager.stop()` shuts down its executor. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
