+1 for removing the default runner. It has always been the Beam user expectation that a runner needs to be selected.
"PortableRunner" isn't a runner (despite its name) - it's a proxy to a runner that the user specifies via job_endpoint. Thanks for cleaning this up! On Thu, Apr 30, 2020 at 10:11 AM Kyle Weaver <[email protected]> wrote: > I'll bite :) Thanks for the feedback everyone! > > On Thu, Apr 30, 2020 at 1:01 PM Robert Bradshaw <[email protected]> > wrote: > >> I filed https://issues.apache.org/jira/browse/BEAM-9860. Any takers? >> >> On Thu, Apr 30, 2020 at 5:49 AM Ismaël Mejía <[email protected]> wrote: >> >>> +1 for A there are zero reasons to have a default runner set by >>> default, being explicit is better as Robert suggests and it resolves >>> the confusion that the user reported. >>> >>> On Wed, Apr 29, 2020 at 10:05 PM Robert Bradshaw <[email protected]> >>> wrote: >>> > >>> > +1, I was actually thinking about this just the other day. >>> PortableRunner should require job_endpoint to be set, and we can have a >>> nice error message directing the explicit use of FlinkRunner for the old >>> behavior. >>> > >>> > On Wed, Apr 29, 2020 at 11:50 AM Kyle Weaver <[email protected]> >>> wrote: >>> >> >>> >> > Could the error message suggest switching to FlinkRunner (and/or >>> other runners that start a job server for you)? Then it seems like the >>> breakage would only be a minor annoyance. >>> >> >>> >> Definitely. >>> >> >>> >> On Wed, Apr 29, 2020 at 2:49 PM Brian Hulette <[email protected]> >>> wrote: >>> >>> >>> >>> Could the error message suggest switching to FlinkRunner (and/or >>> other runners that start a job server for you)? Then it seems like the >>> breakage would only be a minor annoyance. >>> >>> >>> >>> Brian >>> >>> >>> >>> On Wed, Apr 29, 2020 at 11:32 AM Kyle Weaver <[email protected]> >>> wrote: >>> >>>> >>> >>>> Hi all, >>> >>>> >>> >>>> Currently, when running a pipeline that has the options >>> runner=PortableRunner and job_endpoint unset, the Python SDK spins up a >>> Dockerized Flink job server [1]. This is problematic because the >>> PortableRunner can be used by any portable runner. So for example, a Spark >>> runner user was recently baffled when their job ran successfully but >>> printed a bunch of Flink log messages. >>> >>>> >>> >>>> There are not too many uses of this default behavior to my >>> knowledge, at least within Beam itself. The only example I could find was >>> in the portableWordCount tests, which is mostly the same as >>> portableWordCountFlinkRunner tests [2]. The default behavior is entirely >>> superseded by the FlinkRunner class, which provides better encapsulation. >>> >>>> >>> >>>> I also noticed that DockerizedJobServer is only used by [3]. In >>> FlinkRunner, we pull the job server from Maven if necessary and call Java >>> directly. In general, I think there are already quite enough knobs in the >>> portability framework, so we should remove it unless there is reason to >>> prefer running the job server with Docker instead of calling Java directly. >>> >>>> >>> >>>> There are a couple options: >>> >>>> >>> >>>> A) Remove the default behavior and require job_endpoint to always >>> be set when using PortableRunner. This would be a breaking change. >>> >>>> B) Keep the current behavior, but warn when the user sets >>> runner=PortableRunner without job_endpoint. This is easy to miss, but it's >>> better than nothing. >>> >>>> >>> >>>> What do you think? >>> >>>> >>> >>>> [1] >>> https://github.com/apache/beam/blob/33c73739cec8bc6a7c8319efa41eda7a2540bce1/sdks/python/apache_beam/runners/portability/job_server.py#L184 >>> >>>> [2] >>> https://github.com/apache/beam/blob/b3596b89dbc002c686bdaa7853074e757a81b6fb/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L1983-L2048 >>> >>>> [3] >>> https://github.com/apache/beam/blob/33c73739cec8bc6a7c8319efa41eda7a2540bce1/sdks/python/apache_beam/runners/portability/job_server.py#L163 >>> >>
