[GitHub] [beam] kennknowles commented on a change in pull request #13893: [BEAM-11752] Using LoadingCache instead of Map to cache BundleProcessor

[GitBox <gi...@apache.org>]

kennknowles commented on a change in pull request #13893:
URL: https://github.com/apache/beam/pull/13893#discussion_r571262843



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java
##########
@@ -705,6 +718,17 @@ void reset() throws Exception {
         resetFunction.run();
       }
     }
+
+    void shutdown() {
+      for (ThrowingRunnable tearDownFunction : getTearDownFunctions()) {
+        LOG.debug("Tearing down function {}", tearDownFunction);
+        try {
+          tearDownFunction.run();
+        } catch (Exception e) {
+          LOG.error("Failed to call teardown function: {}", e);

Review comment:
       Interesting. I did not find good documentation on the expectation about whether a bundle should fail if `TearDown` fails. The most detailed docs are the [javadoc](https://beam.apache.org/releases/javadoc/2.27.0/org/apache/beam/sdk/transforms/DoFn.Teardown.html) and they do not describe this. That seems like we need to describe the expectation.
   
   Note that I think this will call http://www.slf4j.org/apidocs/org/slf4j/Logger.html#error(java.lang.String,java.lang.Throwable) because it is the most precise overloaded method.
   
   I think that is good because it will give a stack trace.
   
   But the `{}` will not be replaced with anything.




----------------------------------------------------------------
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:
users@infra.apache.org