-
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
Allow to set timeout for finishing a remote bundle in Samza portable runner #25031
Allow to set timeout for finishing a remote bundle in Samza portable runner #25031
Conversation
@xinyuiscool please help take a look, thanks. |
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.
Can we add unit tests?
runners/samza/src/main/java/org/apache/beam/runners/samza/SamzaPortablePipelineOptions.java
Outdated
Show resolved
Hide resolved
runners/samza/src/main/java/org/apache/beam/runners/samza/runtime/SamzaDoFnRunners.java
Outdated
Show resolved
Hide resolved
runners/samza/src/main/java/org/apache/beam/runners/samza/runtime/SamzaDoFnRunners.java
Outdated
Show resolved
Hide resolved
I didn't do that because there is no test cases created for this |
@mynameborat Thanks for helping review. I addressed your comments, let me know if you have other questions. |
runners/samza/src/test/java/org/apache/beam/runners/samza/util/RunWithTimeoutTest.java
Outdated
Show resolved
Hide resolved
runners/samza/src/main/java/org/apache/beam/runners/samza/util/RunWithTimeout.java
Outdated
Show resolved
Hide resolved
7023b18
to
d59a775
Compare
runners/samza/src/main/java/org/apache/beam/runners/samza/util/RunWithTimeout.java
Outdated
Show resolved
Hide resolved
d59a775
to
b058574
Compare
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.
LGTM!
b058574
to
a677df5
Compare
a677df5
to
db7073d
Compare
addresses #25030
We have observed that some UDFs(with bugs or imported some bad third-party libs) could hang the runner and SDK harness processes in Samza portable mode. Both runner and SDK harness processes are in a zombie state and have no data processing in this case, however, the processes are not able to be shut down although Samza runner provides some built-in timeout functionality.
This change will allow the users to set a timeout for closing a remote bundle in Samza portable runner. Once the timeout occurred, the runner and SDK processes will be shut down properly by Samza's built-in task timeout functionality if enabled.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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.