-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Issue#23071] Fix AfterProcessingTime for Python to behave like Java #23100
Conversation
R:@pabloem as we discussed, this is the fix |
| beam.GroupByKey() | ||
| beam.Map(lambda x: x[1])) | ||
|
||
expected = [[i, i + 1] |
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.
Maybe this is overly complicated? and we can just assume a fixed value
Codecov Report
@@ Coverage Diff @@
## master #23100 +/- ##
==========================================
- Coverage 73.58% 73.52% -0.07%
==========================================
Files 716 716
Lines 95301 95454 +153
==========================================
+ Hits 70132 70178 +46
- Misses 23873 23980 +107
Partials 1296 1296
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
please fix lint : ) |
Assigning reviewers. If you would like to opt out of this review, comment R: @pabloem for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
context.set_timer( | ||
'', TimeDomain.REAL_TIME, context.get_current_time() + self.delay) | ||
context.add_state(self.COUNT_TAG, 1) | ||
if context.get_state(self.COUNT_TAG) == 1: |
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.
Would this be the correct check?
if context.get_state(self.COUNT_TAG) == 1: | |
if context.get_state(self.COUNT_TAG) >= 1: |
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 that would make it the same as before, it would increase the timer for every element, we only want to increase if it's the first we see.
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 have modified the logic from a Counter to a State, we don't need to actually count the elements, just modify the timer with the first element in the pane
Run Python PreCommit |
lgtm! |
Fixes #23071
Context and examples to show the issue added there.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.