[GitHub] [airflow] johannaojeling commented on a diff in pull request #28764: Add support for running a Beam Go pipeline with an executable binary

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

johannaojeling commented on code in PR #28764:
URL: https://github.com/apache/airflow/pull/28764#discussion_r1065071532


##########
airflow/providers/apache/beam/operators/beam.py:
##########
@@ -563,8 +582,13 @@ def __init__(
             )
         self.dataflow_support_impersonation = False
 
+        if not bool(go_file) ^ bool(go_binary):

Review Comment:
   Ah nice, will update to use the helper. Thanks!
   
   Regarding the worker binary, I think in production settings in many cases the launcher and worker binaries will be the same artifact, compiled for the same platform. So it might make sense to still have the `worker_binary` parameter as optional, but if not set, default it to the same path as the `go_binary`? That way we can ensure the `--worker_binary` is always passed when launching the pipeline and won't run into that error, but we don't require the user to explicitly set the worker_binary if it has the same value as the go_binary or is not needed at all (e.g. if running with DirectRunner the worker_binary is not needed so it might be strange to require it). Thoughts on this?
   
   I will make a suggestion and also change the GCS download method to only download one object if the launcher and worker binaries have the same URI.



-- 
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: commits-unsubscribe@airflow.apache.org

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