You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/07/02 08:57:01 UTC
[GitHub] [beam] ihji opened a new pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
ihji opened a new pull request #12164:
URL: https://github.com/apache/beam/pull/12164
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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
[GitHub] [beam] ihji commented on pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #12164:
URL: https://github.com/apache/beam/pull/12164#issuecomment-653401889
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
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
[GitHub] [beam] ihji removed a comment on pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
ihji removed a comment on pull request #12164:
URL: https://github.com/apache/beam/pull/12164#issuecomment-653401889
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
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
[GitHub] [beam] ihji commented on pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #12164:
URL: https://github.com/apache/beam/pull/12164#issuecomment-659084291
@chamikaramj @robertwb Any other feedback before we merge this?
----------------------------------------------------------------
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
[GitHub] [beam] ihji commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r455446568
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
@robertwb The only problematic scenario is when `WindowingStrategy` comes from different SDK and the proto doesn't have a value in `environment_id` field. In that case, the host SDK will populate the field with its default environment. But I think it shouldn't happen since it's expansion service's responsibility to pass a semantically correct proto.
----------------------------------------------------------------
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
[GitHub] [beam] chamikaramj commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r455446367
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
We simply use the environment passed here as the default environment for PipelineContext that is used when building the windowing proto: https://github.com/apache/beam/blob/master/sdks/python/apache_beam/runners/dataflow/dataflow_runner.py#L1411
I think x-lang custom windowing fn does not work today. Heejong filed https://issues.apache.org/jira/browse/BEAM-10507. I'll look into this but I think this is less severe and does not affect Kafka.
----------------------------------------------------------------
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
[GitHub] [beam] chamikaramj commented on pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #12164:
URL: https://github.com/apache/beam/pull/12164#issuecomment-654466345
Yes, WindowingStrategy is not directly mapped to a transform and might require a SDK function hence we had to leave the environment_id there.
----------------------------------------------------------------
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
[GitHub] [beam] chamikaramj merged pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
chamikaramj merged pull request #12164:
URL: https://github.com/apache/beam/pull/12164
----------------------------------------------------------------
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
[GitHub] [beam] robertwb commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r455430204
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
No, we can't assume this. I don't know where this is being consumed.
----------------------------------------------------------------
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
[GitHub] [beam] chamikaramj commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r455441379
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
Heejong clarified that what we pass here is just the default.
----------------------------------------------------------------
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
[GitHub] [beam] ihji commented on pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #12164:
URL: https://github.com/apache/beam/pull/12164#issuecomment-653417664
R: @robertwb
----------------------------------------------------------------
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
[GitHub] [beam] chamikaramj commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r451184245
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
Can we always assume default environment here ? What about x-lang WindowingStrategies ? Probably this can be tested by running a x-lang GBK test with Dataflow with a WindowingStrategy.
----------------------------------------------------------------
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
[GitHub] [beam] robertwb commented on a change in pull request #12164: [BEAM-10397] add missing environment in windowing strategy for Dataflow
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #12164:
URL: https://github.com/apache/beam/pull/12164#discussion_r455444790
##########
File path: sdks/python/apache_beam/runners/dataflow/dataflow_runner.py
##########
@@ -864,7 +867,7 @@ def run_GroupByKey(self, transform_node, options):
windowing = transform_node.transform.get_windowing(transform_node.inputs)
step.add_property(
PropertyNames.SERIALIZED_FN,
- self.serialize_windowing_strategy(windowing))
+ self.serialize_windowing_strategy(windowing, self._default_environment))
Review comment:
But the default might not be the right thing here if this is from another language, right? I'd like to better understand where this is being used and what can go wrong if it's missing/incorrect.
----------------------------------------------------------------
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