Timeout execution tree creation for SDK worker ops. - #36200
Conversation
501644c to
0cece5b
Compare
0cece5b to
4fd4824
Compare
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| # a subprocess is launched during the import operation. | ||
| _LOGGER.error( | ||
| 'Timed out when creating execution tree for %s.\n%s', | ||
| self.process_bundle_descriptor.id, |
There was a problem hiding this comment.
since we terminate this sdk harness later, I think this error should be harmless. So can we mention this somehow?
There was a problem hiding this comment.
+1 ", restarting harness to retry and continue processing."
I think you might also want to map this to concepts the user is more familiar with like unpickling and calling their setup method.
"This may indicate issues unpickling or in the dofn setup function."
There was a problem hiding this comment.
ping on this part, I think otherwise we could punt on other comments but this seems good to fix.
There was a problem hiding this comment.
Updated the message, thanks.
| future = executor.submit( | ||
| self.create_execution_tree, self.process_bundle_descriptor) | ||
| try: | ||
| self.ops = future.result(timeout=3600) |
There was a problem hiding this comment.
could this be a pipeline option? an hour might be prohibitively long for some cases. Also since this ends up calling user setup function there is possiblity an hour is too short
I think we likely want to add options for how long to cache these bundle processors as well. Currently it is just 60 seconds and then we clear the cache. If this is expensive for streaming pipelines we should evict less aggressively. (that can be a different cl just bringing it up as motivation for a new options class controlling stuff like this)
There was a problem hiding this comment.
Using an option can be doable, we could tie it to the existing per-element timeout option. Note that DoFn.setup is called a bit later, immediately after execution tree creation. Slow Setup operations will not be causing the timeout here.
There was a problem hiding this comment.
I was mostly worried this might break things with very expensive setup, but since that's not included I think we could punt on making it configurable if you just want to cover the unpickling.
It could be nice to cover the setup with a timeout too. I believe we had issues in the past where setup getting stuck was hard to debug since it is covered by the lull logic. An alternative would be to defer calling setup until after lull registration
There was a problem hiding this comment.
i am looking at some of those issues since this work is related. I might merge this PR as is before next release but still looking to see if we can get a more general solution here
| # a subprocess is launched during the import operation. | ||
| _LOGGER.error( | ||
| 'Timed out when creating execution tree for %s.\n%s', | ||
| self.process_bundle_descriptor.id, |
There was a problem hiding this comment.
+1 ", restarting harness to retry and continue processing."
I think you might also want to map this to concepts the user is more familiar with like unpickling and calling their setup method.
"This may indicate issues unpickling or in the dofn setup function."
| self.counter_factory) | ||
|
|
||
| self.ops = self.create_execution_tree(self.process_bundle_descriptor) | ||
| with concurrent.futures.ThreadPoolExecutor( |
There was a problem hiding this comment.
If we are going to add timeouts in other places that might be called more frequently, I think we would want some more generic facility to register thread and timeout and then have single background thread observing for timeouts instead of starting a thread for each call. This seems ok here but just something to consider for future.
with WatchdogTimeout(3600, handle_timeout):
// run code that might get stuck inline on this thread
There was a problem hiding this comment.
That looks like a neat construct, good idea.
| future = executor.submit( | ||
| self.create_execution_tree, self.process_bundle_descriptor) | ||
| try: | ||
| self.ops = future.result(timeout=3600) |
There was a problem hiding this comment.
I was mostly worried this might break things with very expensive setup, but since that's not included I think we could punt on making it configurable if you just want to cover the unpickling.
It could be nice to cover the setup with a timeout too. I believe we had issues in the past where setup getting stuck was hard to debug since it is covered by the lull logic. An alternative would be to defer calling setup until after lull registration
| # a subprocess is launched during the import operation. | ||
| _LOGGER.error( | ||
| 'Timed out when creating execution tree for %s.\n%s', | ||
| self.process_bundle_descriptor.id, |
There was a problem hiding this comment.
ping on this part, I think otherwise we could punt on other comments but this seems good to fix.
|
@tvalentyn this PR is stale for a week. Please resolve comments and work with reviewer. |
4 participants
In rare cases, unpickling a DoFn might get permanently stuck.
One Beam customer sporadically observed this stuckness in a scenario when unpickling involved importing a module (pygithub?), where the import operation involved launching a subprocess.
This PR adds a 60 min timeout to create bundle processor operations, after which the SDK is terminated. Runner would then restart the work item.
Tested:
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.mdwith 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 or the workflows README to see a list of phrases to trigger workflows.