Abacn commented on code in PR #32522:
URL: https://github.com/apache/beam/pull/32522#discussion_r1771673821


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java:
##########
@@ -1168,13 +1168,27 @@ void discard() {
         if (this.bundleCache != null) {
           this.bundleCache.clear();
         }
+        // setupFunction called in createBundleProcessor when 
BundleProcessorCache.get returns null.
+        // call teardownFunction here as the BundleProcessor is already 
removed from cache and isn't
+        // going to be re-used.
+        for (ThrowingRunnable teardownFunction : 
Lists.reverse(this.getTearDownFunctions())) {
+          try {
+            teardownFunction.run();

Review Comment:
   The naming here were confusing. `startFunction` are for `startBundle`. 
`tearDown` corresponds to `setup` in terms of DoFns, where `setup` was called 
as early as:
   
   ```
   
org.apache.beam.sdk.transforms.reflect.DoFnInvokers.tryInvokeSetupFor(DoFnInvokers.java:53)
   org.apache.beam.fn.harness.FnApiDoFnRunner.<init>(FnApiDoFnRunner.java:505)
   
org.apache.beam.fn.harness.FnApiDoFnRunner$Factory.createRunnerForPTransform(FnApiDoFnRunner.java:195)
   
org.apache.beam.fn.harness.FnApiDoFnRunner$Factory.createRunnerForPTransform(FnApiDoFnRunner.java:163)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:307)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createBundleProcessor(ProcessBundleHandler.java:861)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.lambda$processBundle$0(ProcessBundleHandler.java:511)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler$BundleProcessorCache.get(ProcessBundleHandler.java:972)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.processBundle(ProcessBundleHandler.java:507)
   
   ```
   
   that was when BundleProcessor gets created. Therefore in 
bundleProcessor.discard call DoFn's `@Setup` have been called before.



##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java:
##########
@@ -1168,13 +1168,27 @@ void discard() {
         if (this.bundleCache != null) {
           this.bundleCache.clear();
         }
+        // setupFunction called in createBundleProcessor when 
BundleProcessorCache.get returns null.
+        // call teardownFunction here as the BundleProcessor is already 
removed from cache and isn't
+        // going to be re-used.
+        for (ThrowingRunnable teardownFunction : 
Lists.reverse(this.getTearDownFunctions())) {
+          try {
+            teardownFunction.run();

Review Comment:
   The naming here was confusing. `startFunction` are for `startBundle`. 
`tearDown` corresponds to `setup` in terms of DoFns, where `setup` was called 
as early as:
   
   ```
   
org.apache.beam.sdk.transforms.reflect.DoFnInvokers.tryInvokeSetupFor(DoFnInvokers.java:53)
   org.apache.beam.fn.harness.FnApiDoFnRunner.<init>(FnApiDoFnRunner.java:505)
   
org.apache.beam.fn.harness.FnApiDoFnRunner$Factory.createRunnerForPTransform(FnApiDoFnRunner.java:195)
   
org.apache.beam.fn.harness.FnApiDoFnRunner$Factory.createRunnerForPTransform(FnApiDoFnRunner.java:163)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:307)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createRunnerAndConsumersForPTransformRecursively(ProcessBundleHandler.java:261)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.createBundleProcessor(ProcessBundleHandler.java:861)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.lambda$processBundle$0(ProcessBundleHandler.java:511)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler$BundleProcessorCache.get(ProcessBundleHandler.java:972)
   
org.apache.beam.fn.harness.control.ProcessBundleHandler.processBundle(ProcessBundleHandler.java:507)
   
   ```
   
   that was when BundleProcessor gets created. Therefore in 
bundleProcessor.discard call DoFn's `@Setup` have been called before.



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to