kl0u commented on a change in pull request #10311: [FLINK-14762][client] Enrich
JobClient API
URL: https://github.com/apache/flink/pull/10311#discussion_r351668932
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/deployment/AbstractJobClusterExecutor.java
##########
@@ -66,7 +68,41 @@ public AbstractJobClusterExecutor(@Nonnull final
ClientFactory clusterClientFact
LOG.info("Job has been submitted with JobID " +
jobGraph.getJobID());
final boolean withShutdownHook =
!configAccessor.getDetachedMode() && configAccessor.isShutdownOnAttachedExit();
- return CompletableFuture.completedFuture(new
ClusterClientJobClientAdapter<>(clusterClient, jobGraph.getJobID(),
withShutdownHook));
+
+ if (withShutdownHook) {
+ Thread shutdownHook =
ShutdownHookUtil.addShutdownHook(
+ clusterClient::shutDownCluster,
clusterClient.getClass().getSimpleName(), LOG);
+
+ return CompletableFuture.completedFuture(new
ClusterClientJobClientAdapter<ClusterID>(clusterClient, jobGraph.getJobID()) {
+ @Override
+ public void doClose() {
+ Throwable throwable = null;
+
+ try {
Review comment:
Why not merging the two blocks like:
```
try {
ShutdownHookUtil.removeShutdownHook(shutdownHook,
clusterClient.getClass().getSimpleName(), LOG);
clusterClient.close();
} catch (Throwable t) {
throwable = ExceptionUtils.firstOrSuppressed(t, throwable)
}
if (throwable != null) {
ExceptionUtils.rethrow(throwable);
}
```
Also now the `clusterClient.close()` does not throw an exception so I am not
sure if it is even needed to be in the `try-catch` block.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
