afterincomparableyum commented on code in PR #3724:
URL: https://github.com/apache/celeborn/pull/3724#discussion_r3396992593
##########
cpp/celeborn/client/ShuffleClient.cpp:
##########
@@ -858,6 +904,12 @@ bool ShuffleClientImpl::cleanupShuffle(int shuffleId) {
return true;
}
+void ShuffleClientImpl::shutdown() {
Review Comment:
shutdown() clears only pushExcludedWorkers.
Java clears both exclusion sets. Java's shutdown()
(ShuffleClientImpl.java:2052–2077) also clears fetchExcludedWorkers (and closes
the revive manager, retry pool, client factory).
```
@Override
public void shutdown() {
if (null != reviveManager) {
reviveManager.close();
}
if (null != rpcEnv) {
rpcEnv.shutdown();
}
if (null != dataClientFactory) {
dataClientFactory.close();
}
if (null != transportContext) {
transportContext.close();
}
if (null != pushDataRetryPool) {
pushDataRetryPool.shutdown();
}
if (null != lifecycleManagerRef) {
lifecycleManagerRef = null;
}
shuffleIdCache.clear();
pushExcludedWorkers.clear();
fetchExcludedWorkers.clear();
messagesHelper.close();
logger.warn("Shuffle client has been shutdown!");
}
```
If you want C++ teardown of pools/manager is the destructor's job that's
fine, I would just double check that this is the case. But
fetchExcludedWorkers_ is the same category of state being cleared like Java.
Clear it here too, or note why not.
--
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]
