srowen commented on a change in pull request #25759: [SPARK-19147][CORE] netty
throw NPE
URL: https://github.com/apache/spark/pull/25759#discussion_r323742872
##########
File path:
common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java
##########
@@ -192,7 +192,20 @@ public TransportClient createClient(String remoteHost,
int remotePort)
logger.info("Found inactive connection to {}, creating a new one.",
resolvedAddress);
}
}
- clientPool.clients[clientIndex] = createClient(resolvedAddress);
+ try {
+ clientPool.clients[clientIndex] = createClient(resolvedAddress);
+ } catch (Exception e) {
+ // createClient() is called by task and close() is called by executor.
+ // When stop the executor, close() will set workerGroup = null,
+ // NPE will occur in createClient which generate many exception in log.
+ // For exception occurs after close(), treated it as an expected
Exception
+ // and transform it to InterruptedException which can be processed by
Executor.
+ // See SPARK-19147
+ if (workerGroup == null) {
+ throw new InterruptedException(e.getMessage());
Review comment:
This is still going to generate an exception in the logs, no? should it just
be a log warning?
This is I think too indirect. Why not throw `IllegalStateException` in
`createClient` instead in this case and catch for it specifically?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
