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-11971] Fix directrunner timer consistency #16650

Merged
merged 1 commit into from
Feb 3, 2022
Merged

[BEAM-11971] Fix directrunner timer consistency #16650

merged 1 commit into from
Feb 3, 2022

Conversation

reuvenlax
Copy link
Contributor

@reuvenlax reuvenlax commented Jan 29, 2022 •
edited
Loading

  • Replaces pushedBackTimers with the technique used in FnApiDoFnRunner.
  • Removes buggy use of Concurrent* data structures (those data structures only provide weakly-consistent iteration, which violates assumptions in the DirectRunner)
  • Fix a few bugs associated with looping timers

@reuvenlax reuvenlax force-pushed the fix_directrunner_looping_timer branch 3 times, most recently from 2f4de0b to a8c89ba Compare January 29, 2022 23:00
@reuvenlax
Copy link
Contributor Author

Run Java PreCommit

@reuvenlax
Copy link
Contributor Author

Run Java_PVR_Flink_Batch PreCommit

@reuvenlax reuvenlax changed the title Fix directrunner looping timer [BEAM-11971] Fix directrunner timer consistency Jan 30, 2022
@reuvenlax
Copy link
Contributor Author

R: @rezarokni

@reuvenlax
Copy link
Contributor Author

Run Java PreCommit

@aaltay aaltay requested a review from kennknowles January 31, 2022 19:41
@reuvenlax reuvenlax requested a review from je-ik January 31, 2022 22:42
je-ik
je-ik reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@je-ik je-ik left a comment

Choose a reason for hiding this comment

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

The only thing I'm not sure how is how setting looping timer in earlierTimers should work. Could we maybe test that explicitly with a unit test?

.../direct-java/src/main/java/org/apache/beam/runners/direct/StatefulParDoEvaluatorFactory.java Show resolved Hide resolved
runners/direct-java/src/main/java/org/apache/beam/runners/direct/WatermarkManager.java Outdated
}
}

for (TimerData timer : update.getDeletedTimers()) {
if (TimeDomain.EVENT_TIME.equals(timer.getDomain())) {
String timerKey = timer.getTimerId() + '+' + timer.getTimerFamilyId();
Copy link
Contributor

Choose a reason for hiding this comment

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

The construction of the timerKey seems repeated, could we wrap it in a helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up timerKey usage

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java Show resolved Hide resolved
je-ik
je-ik reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@je-ik je-ik left a comment

Choose a reason for hiding this comment

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

LGTM.
nit: maybe we could slightly improve the stringKey for better readability.

runners/core-java/src/main/java/org/apache/beam/runners/core/TimerInternals.java Show resolved Hide resolved
@reuvenlax
Copy link
Contributor Author

Run Java PreCommit

@lukecwik
Copy link
Member

lukecwik commented Feb 2, 2022

CC: @kileys

je-ik
je-ik approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@je-ik je-ik left a comment

Choose a reason for hiding this comment

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

LGTM

@reuvenlax reuvenlax force-pushed the fix_directrunner_looping_timer branch from 83d976b to 8ea77f9 Compare February 3, 2022 17:17
@reuvenlax reuvenlax merged commit 6814a06 into apache:master Feb 3, 2022
@ibzib
Copy link
Contributor

ibzib commented Feb 7, 2022

@reuvenlax - we rolled this back because several tests were failing #16748

@reuvenlax
Copy link
Contributor Author

reuvenlax commented Feb 8, 2022 via email

@damccorm damccorm mentioned this pull request Jun 4, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@je-ik je-ik je-ik approved these changes

@kennknowles kennknowles Awaiting requested review from kennknowles

Assignees
No one assigned
Labels
core direct java local runners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reuvenlax @lukecwik @ibzib @je-ik