[BEAM-12742] Samza Runner does not properly delete modified timer #15322
Conversation
1. Update logic to fire timer when timestamp equals to watermark 2. Properly set timer family id when constructing TimerData in order to be properly deleted. 3. Get timestamp from state store instead of Instance.now() in order to be properly deleted. 4. Include NeedsRunner test catagory.
|
Dataflow Tests failed because of BEAM-12699 |
|
Run Samza ValidatesRunner |
|
Is this ready for review? LMK if I should help you find a reviewer of if you're good finding one yourself : ) |
|
Thank you @pabloem for the offering. Could you help me identify a reviewer to review the changes on @xinyuiscool would help review the Samza runner changes. |
pabloem
left a comment
pabloem
left a comment
There was a problem hiding this comment.
Added a few comments. LGTM other than that.
|
|
||
| @Test | ||
| @Category(NeedsRunner.class) | ||
| @Category({NeedsRunner.class, UsesTimersInParDo.class, UsesTestStream.class}) |
There was a problem hiding this comment.
Thanks a lot for adding all of these categories. It will be helpful. Maybe add UsesTestStream.class to all of those that also have UsesTestStreamWithProcessingTime.class?
There was a problem hiding this comment.
Good point, this is what I would like to understand the common practice in beam.
UsesTestStreamWithProcessingTime is a subtype of UsesTestStream, based on JUnit documentation, tests marked as UsesTestStreamWithProcessingTime is also part of UsesTestStream but not the other way around.
Therefore, if a test is marked as UsesTestStreamWithProcessingTime, then marking it with UsesTestStream does not effectively change how it behaves since it transiently belongs to UsesTestStream category as well.
However, I do agree that having both annotations here are clearer and more readable and there does not seem to have cons to have both annotations.
What are your suggestions here?
There was a problem hiding this comment.
Updated all tests with UsesTestStreamWithProcessingTime to have UsesTestStream for now.
There was a problem hiding this comment.
hi - well, I am not an expert on JUnit, but we have recently started storing the annotations, and we plan over the long term to use them to build a capability matrix, so I think it's best to separate all the sub-categories. Thanks!
There was a problem hiding this comment.
I have updated the PR so that tests with UsesTestStreamWithProcessingTime are also annotated with UsesTestStream, does this look good to you?
|
Run Java PreCommit |
xinyuiscool
left a comment
xinyuiscool
left a comment
There was a problem hiding this comment.
Changes on Samza Runner looks good!
|
@pabloem Can you help take another look ? |
|
Run Java_Examples_Dataflow PreCommit |
|
Run Java_Examples_Dataflow_Java11 PreCommit |
3 participants
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:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.