[GitHub] [beam] lukecwik commented on a change in pull request #16769: [BEAM-13193] Enable process bundle response elements embedding in Jav…

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

lukecwik commented on a change in pull request #16769:
URL: https://github.com/apache/beam/pull/16769#discussion_r801123126



##########
File path: sdks/java/fn-execution/src/main/java/org/apache/beam/sdk/fn/data/BeamFnDataOutboundAggregator.java
##########
@@ -199,7 +210,14 @@ public void sendBufferedDataAndFinishOutboundStreams() {
           .setIsLast(true);
       entry.getValue().resetStats();
     }
+    if (collectElementsIfNoFlushes && !hasFlushedForBundle) {
+      return bufferedElements.build();
+    }
     outboundObserver.onNext(bufferedElements.build());
+    // This is now at the end of a bundle, so we reset hasFlushedForBundle to prepare for new
+    // bundles.
+    hasFlushedForBundle = false;

Review comment:
       `hasFlushedForBundle` is outside of the `flushLock` and is being read/written by different threads.

##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java
##########
@@ -986,7 +1002,13 @@ void finish() {
       inboundObserver2 =
           BeamFnDataInboundObserver2.forConsumers(getInboundDataEndpoints(), getTimerEndpoints());
       for (BeamFnDataOutboundAggregator aggregator : getOutboundAggregators().values()) {
-        aggregator.startFlushThread();
+        aggregator.start(
+            getRunnerCapabilities()
+                    .contains(
+                        BeamUrns.getUrn(
+                            StandardRunnerProtocols.Enum.CONTROL_RESPONSE_ELEMENTS_EMBEDDING))
+                // TODO: Handle multiple outbound ApiServiceDescriptors.

Review comment:
       ```suggestion
                   // TODO(BEAM-13193): Handle multiple outbound ApiServiceDescriptors.
   ```

##########
File path: sdks/java/fn-execution/src/main/java/org/apache/beam/sdk/fn/data/BeamFnDataOutboundAggregator.java
##########
@@ -85,10 +87,17 @@ public BeamFnDataOutboundAggregator(
     this.processBundleRequestIdSupplier = processBundleRequestIdSupplier;
     this.bytesWrittenSinceFlush = 0L;
     this.flushLock = new Object();
+    this.hasFlushedForBundle = false;
   }
 
-  /** Starts the flushing daemon thread if data_buffer_time_limit_ms is set. */
-  public void startFlushThread() {
+  /**
+   * Starts the flushing daemon thread if data_buffer_time_limit_ms is set. If
+   * collectElementsIfNoFlushes is true, {@link
+   * #sendOrCollectBufferedDataAndFinishOutboundStreams()} returns the buffered data instead of
+   * sending it to the outboundObserver if no flushes have taken place within this bundle
+   */
+  public void start(boolean collectElementsIfNoFlushes) {
+    this.collectElementsIfNoFlushes = collectElementsIfNoFlushes;

Review comment:
       We should know that at construction time since we should know the runner capability before we even start processing a bundle allowing for `collectElementsIfNoFlushes` to be final but this would be difficult with out solving the multiple aggregator case which doesn't seem challenging to capture the results for each aggregator and either embed them in the response if you captured them all or replay them back to the aggregator if one of them returned null.




-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org