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