-
Notifications
You must be signed in to change notification settings - Fork 568
PYTHON-1354 do not set timeout to None when calling execute_async in execute_concurrent #1260
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
base: master
Are you sure you want to change the base?
Conversation
…execute_concurrent The default timeout=_NOT_SET will use the request_timeout specified in the execution_profile. This is not the same as setting the timeout to None. This will instead result in having no timeout which is not ideal. If this behaviour really is wanted, the request_timeout can be set to None in the execution_profile instead.
d49d026 to
02e8270
Compare
|
There is an issue with the CI build that seems unrelated to the change. Can the build be re-triggered please? |
|
Hey @osttra-h-jarrendal , thanks for the ping! I'm assuming your message above refers to the Travis CI build? If that's the case I don't think re-running the build will address the issue: It looks like libev isn't being installed on those runners, which is a bit strange since we don't explicitly install it in our config. I'll take a quick look and see if this is an easy fix but we'll very likely be retiring Travis completely now that we've moved this project to the Apache Software Foundation. We'll be setting up a new CI infra for this project there but that will take a bit of time to get done. In the interim the best answer is probably to run a test build on our old Jenkins infrastructure. I just tried to kick that off for you but it looks like that's also borked, apparently as a side effect of the move. I'm working on getting all of that resolved but given the holidays it might take a while. |
|
@absurdfarce thank you! |
|
Follow up to my previous comment: now that donation is complete integration with Travis CI has been removed for this repo. We'll setup a public CI which will provide the same functionality so at this point there are no plans to debug the test failures referenced above. We do still owe you a proper review @osttra-h-jarrendal; I'll try to get to that soon. |
| self._exec_depth += 1 | ||
| try: | ||
| future = self.session.execute_async(statement, params, timeout=None, execution_profile=self._execution_profile) | ||
| future = self.session.execute_async(statement, params, execution_profile=self._execution_profile) |
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.
Agree with the analysis here. execute_async() has a default value for the timeout param which results in the default timeout being used as long as no override is supplied (here by way of this call). Code path in _create_response_future() is slightly different when execution contexts are in play but the same analysis seems to hold.
absurdfarce
left a comment
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'd like to see a full CI run of this but I'm not expecting any significant regressions. INFRA-27454 is still a problem there but we're working on trying to get that resolved.
|
Ping @millerjp as well for visibility. |
When timeout is not passed to execute_async, the default timeout=_NOT_SET will use the request_timeout specified in the execution_profile. This is not the same as setting the timeout to None. This will instead result in having no timeout which is not ideal. If this behavior really is wanted, the request_timeout can be set to None in the execution_profile instead.
Bug ticket: https://datastax-oss.atlassian.net/browse/PYTHON-1354