[GitHub] [beam] ibzib commented on a change in pull request #14923: [BEAM-12419] Add Timeout and WaitForReady support in java gRPC calls

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

ibzib commented on a change in pull request #14923:
URL: https://github.com/apache/beam/pull/14923#discussion_r644169234



##########
File path: runners/portability/java/src/main/java/org/apache/beam/runners/portability/PortableRunner.java
##########
@@ -201,12 +207,16 @@ public PipelineResult run(Pipeline pipeline) {
               .setPreparationId(prepareJobResponse.getPreparationId())
               .build();
 
-      RunJobResponse runJobResponse = jobService.run(runJobRequest);
+      // Run the job and wait for a result, we don't set a timeout here because
+      // it may take a long time for a job to complete and streaming
+      // jobs currently never return a response.

Review comment:
       Nit: wording
   ```suggestion
         // jobs never return a response.
   ```

##########
File path: runners/portability/java/src/main/java/org/apache/beam/runners/portability/PortableRunner.java
##########
@@ -201,12 +207,16 @@ public PipelineResult run(Pipeline pipeline) {
               .setPreparationId(prepareJobResponse.getPreparationId())
               .build();
 
-      RunJobResponse runJobResponse = jobService.run(runJobRequest);
+      // Run the job and wait for a result, we don't set a timeout here because
+      // it may take a long time for a job to complete and streaming
+      // jobs currently never return a response.
+      RunJobResponse runJobResponse = jobService.withDeadline(null).run(runJobRequest);

Review comment:
       Why do we need to add `withDeadline(null)` here? Isn't it a no-op?

##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/options/PortablePipelineOptions.java
##########
@@ -34,6 +34,17 @@
 
   void setJobEndpoint(String endpoint);
 
+  @Description(
+      "Job service request timeout in seconds. The timeout "
+          + "determines the max time the driver program will wait to "
+          + "get a response from the job server. NOTE: the timeout does not "
+          + "apply to the actual pipeline run time. The driver program can "

Review comment:
       Nit: wording
   ```suggestion
             + "apply to the actual pipeline run time. The driver program will "
   ```

##########
File path: runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/ExternalEnvironmentFactory.java
##########
@@ -113,6 +113,7 @@ public RemoteEnvironment createEnvironment(Environment environment, String worke
         ManagedChannelFactory.createDefault().forDescriptor(externalPayload.getEndpoint());
     BeamFnApi.StartWorkerResponse startWorkerResponse =
         BeamFnExternalWorkerPoolGrpc.newBlockingStub(managedChannel)
+            .withWaitForReady()

Review comment:
       What does `withWaitForReady` change, and why do we need it?
   
   Also, I don't think this change belongs in the same PR as the job server changes. Where possible, PRs should do exactly one thing.




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