-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[Prism] Add previousInput watermark and use it in bundleReady #36137
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.
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
[Prism] Add previousInput watermark and use it in bundleReady #36137
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36137 +/- ##
============================================
- Coverage 56.85% 56.84% -0.01%
Complexity 3385 3385
============================================
Files 1220 1220
Lines 185400 185404 +4
Branches 3520 3520
============================================
- Hits 105405 105401 -4
- Misses 76658 76670 +12
+ Partials 3337 3333 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lostluck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #36133 is what I was originally thinking of, but I much prefer this. It's inline with the other watermark tracking fields, and doesn't have a bit going back and forth, which gave me pause. We are checking a property of the watermarks with respect to the other data which is inline with the rest of the ready check.
#36128 (comment) is a good find, and this change satisfies the most important property: All the currently passing tests still pass!
Are there any additional tests we can enable that now pass? (no worries if nothing looks promising at this stage, I know there's additional work in progress that do need this case).
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go
Outdated
Show resolved
Hide resolved
We only change the timing when a bundle is claimed to be ready in certain circumstance. The window pane behavior is not changed in this PR. I am going to look into the extra window pane in the next one. |
796641e to
17cad7d
Compare
|
Assigning reviewers: R: @lostluck for label go. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
A different approach of #36133.
addresses #36128