kerwin-zk commented on PR #13283: URL: https://github.com/apache/gluten/pull/13283#issuecomment-6207035162
Blocking: the new regression test is not hermetic, and it does not actually verify shutdown. [`testFetchCleanFailedShuffleReturnsCleanerInstance`](https://github.com/apache/gluten/blob/2a313e07713a5f77022b660cd05c9cf85eddc86a/gluten-celeborn/src/test/java/org/apache/spark/shuffle/gluten/celeborn/CelebornUtilsTest.java#L124-L140) constructs a real `LifecycleManager`. Its initialization creates an `ApplicationHeartbeater`, so this test performs RPCs against the configured Celeborn master. In my targeted run, all 9 tests passed, but the suite took 10.93 seconds and logged repeated master-connection failures. That makes the unit test depend on ambient configuration and external network availability, which can make it slow or flaky. Also, line 134 only calls `stopFailedShuffleCleaner(cleaner)`; there is no assertion that the cleaner scheduled executor is actually shut down or terminated. If either assertion before that line fails, the cleaner is not stopped because it is not covered by the `finally` block. Could we isolate this test from any external Celeborn master, always clean up the cleaner in `finally`, and assert an observable shutdown state of its executor? The production ownership fix itself looks good. -- 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]
