This is an automated email from the ASF dual-hosted git repository. mxm pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/beam.git
commit 701c4be8436a77a75a431b4c213e8fd85f17c8c3 Merge: b0c1d8c b716151 Author: Maximilian Michels <[email protected]> AuthorDate: Thu Dec 12 13:03:12 2019 +0100 Merge pull request #10345: [BEAM-8943] Cleanup servers in the presence of environment failures .../control/DefaultJobBundleFactory.java | 29 ++++++++++++---------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --cc runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java index a3f6f01,8fb3e87..35146c6 --- a/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java +++ b/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java @@@ -374,17 -374,18 +374,24 @@@ public class DefaultJobBundleFactory im return serverInfo; } - @Override - public void close() throws Exception { - try (AutoCloseable envCloser = environment) { - // Wrap resources in try-with-resources to ensure all are cleaned up. - } - try (AutoCloseable stateServer = serverInfo.getStateServer(); + public void close() { ++ // DO NOT ADD ANYTHING HERE WHICH MIGHT CAUSE THE BLOCK BELOW TO NOT BE EXECUTED. ++ // If we exit prematurely (e.g. due to an exception), resources won't be cleaned up properly. ++ // Please make an AutoCloseable and add it to the try statement below. + try (AutoCloseable envCloser = environment; + AutoCloseable stateServer = serverInfo.getStateServer(); AutoCloseable dateServer = serverInfo.getDataServer(); AutoCloseable controlServer = serverInfo.getControlServer(); AutoCloseable loggingServer = serverInfo.getLoggingServer(); AutoCloseable retrievalServer = serverInfo.getRetrievalServer(); - AutoCloseable provisioningServer = serverInfo.getProvisioningServer()) {} + AutoCloseable provisioningServer = serverInfo.getProvisioningServer()) { + // Wrap resources in try-with-resources to ensure all are cleaned up. ++ // This will close _all_ of these even in the presence of exceptions. ++ // The first exception encountered will be the base exception, ++ // the next one will be added via Throwable#addSuppressed. + } catch (Exception e) { + LOG.warn("Error cleaning up servers {}", environment.getEnvironment(), e); + } // TODO: Wait for executor shutdown? }
