-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add a pipeline update compatibility version option. #29140
Conversation
This can be used to migrate to best practices and good default with new versions of Beam while still allowing users of older SDKs to update their SDK version without breaking update compatibility. Also add the mechanisms to propagate this option for cross-language transforms.
R: @kennknowles |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
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.
+1000000 for this concept. Worth a dev@ thread to gather ideas and advertise this new thing, and since it'll change how we develop in many cases.
|
||
// (Optional) A set of Pipeline Options that should be used | ||
// when expanding this transform. | ||
google.protobuf.Struct pipeline_options = 5; |
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.
@chamikaramj just checking this with you
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.
+1. We've been thinking about propagating other PipelineOptions to expansion as well (which we can piggy-back on top of this feature, on a case-by-case basis).
|
||
@Description( | ||
"If set, attempts to produce a pipeline compatible with this prior version of the Beam SDK." | ||
+ " See https://cloud.google.com/dataflow/docs/guides/updating-a-pipeline.") |
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.
I think we actually have some discussions on dev@ and this also is Flink and Samza. Makes sense, basically. It isn't rigorously defined, but we do OK with it. And I think it is more or less runner independent.
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.
Agreed. Started a discussion .
request.pipeline_options) | ||
# TODO(https://github.com/apache/beam/issues/20090): Figure out the | ||
# correct subset of options to apply to expansion. | ||
if request_options.view_as( |
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.
same comment re: being not just for GCP
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.
I put it here as it's next to the other update options. Open to suggestions if there's a better place. (A new update options?)
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.
Fixed.
+1, this is great. Should we add this to the non-portable PipelineOptions as well? |
And one more technical question - should the version be simply string (that would be compared lexicographically), or should we parse it somehow so that all usages of the version will not re-interpret it? I.e. there could be some issues when (and if) we reach beam 2.100.0. |
I'm intending the version to be compared per https://semver.org/ ; I'll update the docs. As for placement, maybe we should put it in https://beam.apache.org/releases/javadoc/current/index.html?org/apache/beam/sdk/options/StreamingOptions.html I think I'll do that. |
I've resolved the merge conflicts. Please take another look. |
For the record, dev discussion at https://lists.apache.org/thread/29r3zv04n4ooq68zzvpw6zm1185n59m2 In summary, we could do better if we add the capability to inspect the graph at construction time, but that doesn't preclude this step forward for now. It doesn't seem there's any objection to getting this in. |
This can be used to migrate to best practices and good default with new versions of Beam while still allowing users of older SDKs to update their SDK version without breaking update compatibility. Also add the mechanisms to propagate this option for cross-language transforms.
This can be used to migrate to best practices and good default with new versions of Beam while still allowing users of older SDKs to update their SDK version without breaking update compatibility. Also add the mechanisms to propagate this option for cross-language transforms.
This can be used to migrate to best practices and good default with new versions of Beam while still allowing users of older SDKs to update their SDK version without breaking update compatibility.
Also add the mechanisms to propagate this option for cross-language transforms.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.