Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BEAM-97] Input watermarks can never be null #30

Closed
wants to merge 5 commits into from
Closed

[BEAM-97] Input watermarks can never be null #30

wants to merge 5 commits into from

Conversation

mshields822
Copy link
Contributor

No description provided.

@mshields822
Copy link
Contributor Author

R=@kennknowles

@mshields822
Copy link
Contributor Author

This also includes some additional comments and asserts related to pairing output watermark holds with their cleanup timers. They could be separated but since everything is around watermark hold robustness I think we should keep them together.

@mshields822
Copy link
Contributor Author

Hold off on this one please.

@mshields822
Copy link
Contributor Author

@kennknowles
To to reiterate - this one need to wait for a corresponding Google worker cl, and that needs to wait for some backward compat tests to go through.

@mshields822
Copy link
Contributor Author

@kennknowles
This is now ready to go in.

kennknowles
kennknowles reviewed Mar 24, 2016
View reviewed changes
sdk/src/main/java/com/google/cloud/dataflow/sdk/util/TriggerRunner.java
@@ -172,6 +172,8 @@ public boolean shouldFire(W window, Timers timers, StateAccessor<?> state) throw
}

public void onFire(W window, Timers timers, StateAccessor<?> state) throws Exception {
// shouldFire should be false.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? I think rootTrigger.invokeShouldFire(context) should be true, otherwise it is forbidden to call invokeOnFire.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@kennknowles
Copy link
Member

Nice. This makes things so much cleaner.

@mshields822
Copy link
Contributor Author

PTAL

@mshields822
Copy link
Contributor Author

Oh, I'm too late for the rename - will need to rebase.

@mshields822
Copy link
Contributor Author

All good.

@asfgit asfgit closed this in 49d82ba Mar 25, 2016
davorbonaci added a commit to GoogleCloudPlatform/DataflowJavaSDK that referenced this pull request Mar 25, 2016
@davorbonaci
Merge pull request #171 from dhalperi/backport-30
339eb0e 
Backport apache/beam#30
echauchot added a commit to echauchot/beam that referenced this pull request May 12, 2017
@echauchot
Fix query5: Add comment on key lifting
962919d 
issue apache#30
cosmoskitten pushed a commit to cosmoskitten/beam that referenced this pull request Jun 16, 2017
@echauchot
Improve query5, query10 and query11
39f51ba 
query5: Add comment on key lifting (issue apache#30)

query10: Add comment for strange groupByKey (issue apache#31)

query11: Replace Count.perKey by Count.perElement (issue apache#32)
asfgit pushed a commit that referenced this pull request Aug 23, 2017
@echauchot @iemejia
Improve query5, query10 and query11
7bfc982 
query5: Add comment on key lifting (issue #30)

query10: Add comment for strange groupByKey (issue #31)

query11: Replace Count.perKey by Count.perElement (issue #32)
lukecwik referenced this pull request in lukecwik/incubator-beam Mar 22, 2018
@bsidhom
Merge pull request #30 from bsidhom/control-client-pool
0582f9a 
Add ControlClientPool interface and fix broken JobResourceManagerTest
robertwb pushed a commit to robertwb/incubator-beam that referenced this pull request Apr 30, 2020
@kgabryje @robertwb
Rewrite github_sample to 1 line (apache#30)
02232bf 
* Rewrite github_sample to 1 line

* Add license to table shortcode
hengfengli referenced this pull request in hengfengli/beam Mar 21, 2022
@zoercai
refactor: QueryChangeStreamAction (#30)
73d2c3c
ajothomas referenced this pull request in ajothomas/beam Jun 27, 2022
@ajothomas
Wiring Distribution metrics as samza gauges (#30)
95616f6
sjvanrossum pushed a commit to sjvanrossum/beam that referenced this pull request May 17, 2023
@laysakura
Merge pull request apache#30 from laysakura/feat/serde-Coder
f8ad327 
feat: serialize/deserialize custom coders
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
@HemangChothani
docs: document admin client (apache#174)
f099736 
Closes: apache#30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mshields822 @kennknowles