New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-8396] Restore Flink runner LOOPBACK default. #9945
Conversation
Run Python PreCommit |
Run Python2_PVR_Flink PreCommit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Restore" would imply that it was working before and broke. I don't think that is the case. In any case, thanks for the PR!
I have a question about the new parameter. Looks good otherwise.
portable_options = options.view_as(pipeline_options.PortableOptions) | ||
if (options.view_as(FlinkRunnerOptions).flink_master in MAGIC_HOST_NAMES | ||
and not portable_options.environment_type | ||
and not portable_options.output_executable_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertwb What about the case where we have a pre-configured Flink job server with a custom master url? This will then force loopback execution although the user probably does not want that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fair to assume that a pre-configured job server (with a flink_master address) is not possible when using the FlinkRunner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The way to use a pre-configured job server (Flink or other) is to use PortableRunner. This FlinkRunner class is all about automatically doing the setup for you, if needed.
(And, yeah, the old version had a bug...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Now we just need to backport this: #9952
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we conclude the discussion on the ML before merging changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this change was proposed by you and Robert. So it seemed fair to go ahead with it.
portable_options = options.view_as(pipeline_options.PortableOptions) | ||
if (options.view_as(FlinkRunnerOptions).flink_master in MAGIC_HOST_NAMES | ||
and not portable_options.environment_type | ||
and not portable_options.output_executable_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertwb What about the case where we have a pre-configured Flink job server with a custom master url? This will then force loopback execution although the user probably does not want that.
portable_options = options.view_as(pipeline_options.PortableOptions) | ||
if (options.view_as(FlinkRunnerOptions).flink_master in MAGIC_HOST_NAMES | ||
and not portable_options.environment_type | ||
and not portable_options.output_executable_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fair to assume that a pre-configured job server (with a flink_master address) is not possible when using the FlinkRunner.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.