You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@beam.apache.org by GitBox <gi...@apache.org> on 2020/03/18 19:38:00 UTC
[GitHub] [beam] lukecwik opened a new pull request #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
lukecwik opened a new pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162
------------------------
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 | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-600917167
Run Portable_Python 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
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395213529
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -1355,6 +1355,10 @@ message StandardRequirements {
// This requirement indicates the requires_time_sorted_input field of ParDo
// transform payloads must be inspected.
REQUIRES_TIME_SORTED_INPUT = 3 [(beam_urn) = "beam:requirement:pardo:time_sorted_input:v1"];
+
+ // This requirement indicates the restriction_coder_id field of ParDo
+ // transform payloads must be inspected.
+ REQUIRES_SPLITTABLE_DOFN = 4 [(beam_urn) = "beam:requirement:pardo:splittable_dofn:v1"];
Review comment:
Is this a requirement, given that the pipeline would still execute correctly were expansion not performed? (Maybe the performance is bad enough to merit this, but then again, maybe SDF is so primitive that all runners should do something do support sources.)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395269373
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -1355,6 +1355,10 @@ message StandardRequirements {
// This requirement indicates the requires_time_sorted_input field of ParDo
// transform payloads must be inspected.
REQUIRES_TIME_SORTED_INPUT = 3 [(beam_urn) = "beam:requirement:pardo:time_sorted_input:v1"];
+
+ // This requirement indicates the restriction_coder_id field of ParDo
+ // transform payloads must be inspected.
+ REQUIRES_SPLITTABLE_DOFN = 4 [(beam_urn) = "beam:requirement:pardo:splittable_dofn:v1"];
Review comment:
Fair point.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395212786
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -457,10 +457,9 @@ message ParDoPayload {
// be placed in the pipeline requirements.
map<string, TimerFamilySpec> timer_family_specs = 9;
- // Whether the DoFn is splittable
- bool splittable = 6;
-
- // (Required if splittable == true) Id of the restriction coder.
+ // (Optional) Only set when this ParDo contains a splittable DoFn.
Review comment:
I'm not convinced that using the presence of a restriction coder is better than explicitly marking things as splittable, but could possibly be persuaded.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] robertwb commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395269363
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -457,10 +457,9 @@ message ParDoPayload {
// be placed in the pipeline requirements.
map<string, TimerFamilySpec> timer_family_specs = 9;
- // Whether the DoFn is splittable
- bool splittable = 6;
-
- // (Required if splittable == true) Id of the restriction coder.
+ // (Optional) Only set when this ParDo contains a splittable DoFn.
Review comment:
Because it's not as obvious. But I can see the pros and cons.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-601331123
Run Java 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-601292658
Run Java 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-601248628
Run Java 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-600901398
Run Portable_Python 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-601394987
Run Java 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik merged pull request #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-600821917
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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on issue #11162: [BEAM-9339,
BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on issue #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#issuecomment-600910385
Run Portable_Python 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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395216164
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -1355,6 +1355,10 @@ message StandardRequirements {
// This requirement indicates the requires_time_sorted_input field of ParDo
// transform payloads must be inspected.
REQUIRES_TIME_SORTED_INPUT = 3 [(beam_urn) = "beam:requirement:pardo:time_sorted_input:v1"];
+
+ // This requirement indicates the restriction_coder_id field of ParDo
+ // transform payloads must be inspected.
+ REQUIRES_SPLITTABLE_DOFN = 4 [(beam_urn) = "beam:requirement:pardo:splittable_dofn:v1"];
Review comment:
If its unbounded, a runner will never be able to execute it without expansion which requires the runner to at least inspect whether its capable of doing it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [beam] lukecwik commented on a change in pull request #11162:
[BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload,
add splittable dofn requirement to Python SDK.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11162: [BEAM-9339, BEAM-2939] Drop splittable field from ParDoPayload, add splittable dofn requirement to Python SDK.
URL: https://github.com/apache/beam/pull/11162#discussion_r395220391
##########
File path: model/pipeline/src/main/proto/beam_runner_api.proto
##########
@@ -457,10 +457,9 @@ message ParDoPayload {
// be placed in the pipeline requirements.
map<string, TimerFamilySpec> timer_family_specs = 9;
- // Whether the DoFn is splittable
- bool splittable = 6;
-
- // (Required if splittable == true) Id of the restriction coder.
+ // (Optional) Only set when this ParDo contains a splittable DoFn.
Review comment:
`splittable` is invalid without the other field being set so why have it?
It would allow for bugs where one is set and the other is not.
----------------------------------------------------------------
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
With regards,
Apache Git Services