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/10/29 15:05:18 UTC
[GitHub] [beam] scwhittle opened a new pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
scwhittle opened a new pull request #13221:
URL: https://github.com/apache/beam/pull/13221
is used for the state namespace.
Previously prefetching was fetching non-existent state, possibly adding
unnecessary fetches as well as neglecting to prefetch actual state used.
Another way to accomplish this would be to change the TriggerStateMachine
prefetch methods to take a TriggerContext instead of a raw state accessor.
This would allow for TriggerStateMachine to get the correct state for
subtriggers. That seems more inline with how the nonprefetch methods work
however that is a public interface change.
**Please** add a meaningful description for your change here
------------------------
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://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/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_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_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_Direct/lastCompletedBuild/) | --- | [](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 | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
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/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_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.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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] kennknowles commented on a change in pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #13221:
URL: https://github.com/apache/beam/pull/13221#discussion_r516896813
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/triggers/AfterWatermarkStateMachine.java
##########
@@ -91,6 +91,13 @@ public AfterWatermarkEarlyAndLate withLateFirings(TriggerStateMachine lateTrigge
return new AfterWatermarkEarlyAndLate(earlyTrigger, lateTrigger);
}
+ @Override
+ public void prefetchOnElement(PrefetchContext c) {
+ for (ExecutableTriggerStateMachine subTrigger : c.trigger().subTriggers()) {
Review comment:
This was the other one. Same problem with my thinking.
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/triggers/AfterEachStateMachine.java
##########
@@ -48,6 +48,13 @@ private AfterEachStateMachine(List<TriggerStateMachine> subTriggers) {
checkArgument(subTriggers.size() > 1);
}
+ @Override
+ public void prefetchOnElement(PrefetchContext c) {
+ for (ExecutableTriggerStateMachine subTrigger : c.trigger().subTriggers()) {
Review comment:
This was one of the cases where I was thinking it only made sense to prefetch the active subtrigger. Of course, I was missing the whole point that you can't know which one that is until you do a fetch. So maybe my whole idea was foolish.
----------------------------------------------------------------
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] kennknowles merged pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
kennknowles merged pull request #13221:
URL: https://github.com/apache/beam/pull/13221
----------------------------------------------------------------
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] scwhittle commented on a change in pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #13221:
URL: https://github.com/apache/beam/pull/13221#discussion_r516913548
##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/triggers/AfterEachStateMachine.java
##########
@@ -48,6 +48,13 @@ private AfterEachStateMachine(List<TriggerStateMachine> subTriggers) {
checkArgument(subTriggers.size() > 1);
}
+ @Override
+ public void prefetchOnElement(PrefetchContext c) {
+ for (ExecutableTriggerStateMachine subTrigger : c.trigger().subTriggers()) {
Review comment:
Yeah when I embarked on this I was thinking also it might be possible to do better but it didn't pan out.
One idea I had was adding a isCached() method on the state objects that would indicate if the value was local (cached, in-memory, etc) and didn't require a fetch. In such cases we could trim the trigger state to prefetch by looking at the closed triggers. However if everything we actually needed was local, we've only called readLater on something we never actually read and thus never issue a fetch anyway. And if we do have to issue a fetch for anything, fetching an additional unused tag to it is likely not much more overhead.
----------------------------------------------------------------
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] kennknowles commented on pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #13221:
URL: https://github.com/apache/beam/pull/13221#issuecomment-720720140
Oh, yes, the whole module is OK to change. It is just for core runners (those that are in this repo and can be adjusted if needed).
----------------------------------------------------------------
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] scwhittle commented on pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #13221:
URL: https://github.com/apache/beam/pull/13221#issuecomment-720688321
----------------------------------------------------------------
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] scwhittle commented on pull request #13221: [BEAM-11144] Fix trigger prefetching so that the correct trigger index
Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #13221:
URL: https://github.com/apache/beam/pull/13221#issuecomment-718818218
R: @kennknowles Let me know if it is safe to change the parameter for TriggerStateMachine.prefetch* to a TriggerContext. If so If so I think fixing it that way would be more consistent with how the non-prefetch methods access state.
----------------------------------------------------------------
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