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/05/04 03:23:14 UTC
[GitHub] [beam] tudorm opened a new pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
tudorm opened a new pull request #11598:
URL: https://github.com/apache/beam/pull/11598
Such iterables can have enormous sizes and size estimates would cause reading the data multiple times. Furthermore, the size of the entire group-by-key collection is already known precisely at read time (and encoded into corresponding counters), which means the estimation is in fact not only expensive but redundant and unnecessary.
------------------------
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_ValidatesRunner_Flink_Java11/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
[GitHub] [beam] lukecwik commented on pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623537884
How will this impact PCollection size estimation shown in the Dataflow UI?
----------------------------------------------------------------
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] lukecwik commented on pull request #11598: [BEAM-9877] Estimate sizes of group-by-key values behind a key lazily only.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623619474
retest this please
----------------------------------------------------------------
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] ibzib commented on a change in pull request #11598: [BEAM-9877] Estimate sizes of group-by-key values behind a key lazily only.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #11598:
URL: https://github.com/apache/beam/pull/11598#discussion_r419649174
##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/BatchGroupAlsoByWindowViaIteratorsFn.java
##########
@@ -165,12 +168,17 @@ public WindowReiterable(
}
@Override
- public Reiterator<V> iterator() {
+ public WindowReiterator<V> iterator() {
Review comment:
It's odd that `ElementByteSizeObservableIterable::iterator` adds observers within the method body. I assume this is for historic reasons, since it doesn't seem to do anything now, and the comment documenting references a `setObserver` method that doesn't exist. Anyway, your change looks fine. But we should consider cleaning this up.
https://github.com/apache/beam/blob/6453e859badcb629ae2528b77d84235b7291ff89/sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ElementByteSizeObservableIterable.java#L49-L61
----------------------------------------------------------------
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] tudorm commented on pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
Posted by GitBox <gi...@apache.org>.
tudorm commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623240409
R: @kennknowles
R: @lukecwik
----------------------------------------------------------------
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] tudorm commented on pull request #11598: [BEAM-9877] Estimate sizes of group-by-key values behind a key lazily only.
Posted by GitBox <gi...@apache.org>.
tudorm commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623673128
LGTM
----------------------------------------------------------------
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] lukecwik commented on pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623537450
retest this please
----------------------------------------------------------------
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] tudorm commented on pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
Posted by GitBox <gi...@apache.org>.
tudorm commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623600578
Updated the change to perform lazy estimation instead of side-stepping the estimation altogether.
----------------------------------------------------------------
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] tudorm commented on pull request #11598: Do not attempt to estimate sizes of group-by-key values behind a key.
Posted by GitBox <gi...@apache.org>.
tudorm commented on pull request #11598:
URL: https://github.com/apache/beam/pull/11598#issuecomment-623554386
In general, the Dataflow UI ought to use the exact counter GBK has for
effecting the sanity checking, if it doesn't do that already. However, I
believe the estimate in this case is for the PCollection output one step
down from the read from shuffle, at which point I don't know how useful any
estimation is at all when reading from a GBK (except for accounting the
amount of data read and possibly reduced / transformed).
On Mon, May 4, 2020 at 8:38 AM Lukasz Cwik <no...@github.com> wrote:
> How will this impact PCollection size estimation shown in the Dataflow UI?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/11598#issuecomment-623537884>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABRMIED6PZHHSF32BM24HSLRP3OKTANCNFSM4MYNL2QA>
> .
>
----------------------------------------------------------------
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