PYTHON-1354: do not set timeout to None when calling execute_async in execute_concurrent - #972
PYTHON-1354: do not set timeout to None when calling execute_async in execute_concurrent#972mykaul wants to merge 1 commit into
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. patch by osttra-h-jarrendal; reviewed by Bret McGuire and Brad Schoening
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
cassandra/concurrent.py-146-146 (1)
146-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a regression test for the timeout behavior.
Add a test that configures
request_timeoutand verifies that concurrent execution uses it. Also verify that explicitly passingtimeout=Nonestill disables timeouts.As per coding guidelines: “Add relevant tests for new features and bug fixes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/concurrent.py` at line 146, Add regression coverage for the concurrent execution path around session.execute_async, configuring request_timeout and asserting it is used when no per-call timeout is supplied. Also assert that explicitly passing timeout=None disables the configured timeout, preserving the existing override behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@cassandra/concurrent.py`:
- Line 146: Add regression coverage for the concurrent execution path around
session.execute_async, configuring request_timeout and asserting it is used when
no per-call timeout is supplied. Also assert that explicitly passing
timeout=None disables the configured timeout, preserving the existing override
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: f91697ba-197a-4309-a26d-edf063fb3919
📒 Files selected for processing (1)
cassandra/concurrent.py
Summary
_ConcurrentExecutor._executehardcodedtimeout=Nonewhen callingexecute_async, which disables the per-request timeout entirely instead of falling back to therequest_timeoutconfigured on the execution profile. This silently changes the timeout behavior for anyone usingexecute_concurrent/execute_concurrent_with_args.timeout=Noneoverride so the execution profile'srequest_timeoutis honored, matching the documented behavior ofexecute_async.Test plan
master(conflicted with the local synchronous-errback recursion guard incassandra/concurrent.py; resolved by keeping the local recursion-guard structure and only removing thetimeout=Noneargument)tests/unit/test_concurrent.py— 10/10 passedtests/unit(excluding unrelated cython/column_encryption import errors caused by missing optional deps in this sandbox) — 749 passed, 40 skipped