CASSPYTHON-13: Remove eventlet, gevent and twisted event loops - #969
CASSPYTHON-13: Remove eventlet, gevent and twisted event loops#969mykaul wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change removes Eventlet, gevent, and Twisted reactor support from runtime selection, benchmarks, tests, and documentation. Thread pools and queues now use standard-library implementations. Cloud configuration and SSL tests use built-in Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
docs/installation.rst-172-174 (1)
172-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
pyasyncoreinstallation for Python 3.12+.
pip install scylla-driverdoes not installpyasyncore. Add a conditional dependency or documentpython -m pip install pyasyncorebeside the driver installation command.🤖 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 `@docs/installation.rst` around lines 172 - 174, Update the Python 3.12+ installation guidance in the asyncore paragraph to explicitly instruct users to install pyasyncore separately, using the existing driver installation instructions and a command such as python -m pip install pyasyncore; keep the note scoped to versions where asyncore is absent.
🤖 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.
Inline comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 97-102: Harden the extraction flow in the secure-bundle handling
code around ZipFile.extractall by resolving every archive member destination and
rejecting any path outside tmp_dir before extraction. Preserve normal extraction
and parse_cloud_config behavior for safe entries, and add a regression test
covering a traversal archive member.
---
Other comments:
In `@docs/installation.rst`:
- Around line 172-174: Update the Python 3.12+ installation guidance in the
asyncore paragraph to explicitly instruct users to install pyasyncore
separately, using the existing driver installation instructions and a command
such as python -m pip install pyasyncore; keep the note scoped to versions where
asyncore is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 380ff746-7fd2-4186-a4b6-7db1ee384a86
📒 Files selected for processing (26)
benchmarks/base.pycassandra/cluster.pycassandra/connection.pycassandra/datastax/cloud/__init__.pycassandra/datastax/insights/reporter.pycassandra/io/eventletreactor.pycassandra/io/geventreactor.pycassandra/io/twistedreactor.pydocs/api/cassandra/io/eventletreactor.rstdocs/api/cassandra/io/geventreactor.rstdocs/api/cassandra/io/twistedreactor.rstdocs/api/index.rstdocs/installation.rstdocs/security.rsttests/__init__.pytests/integration/long/test_ssl.pytests/integration/standard/test_connection.pytests/unit/io/eventlet_utils.pytests/unit/io/gevent_utils.pytests/unit/io/test_asyncioreactor.pytests/unit/io/test_asyncorereactor.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_geventreactor.pytests/unit/io/test_libevreactor.pytests/unit/io/test_twistedreactor.pytests/unit/io/utils.py
💤 Files with no reviewable changes (16)
- docs/api/cassandra/io/eventletreactor.rst
- docs/api/cassandra/io/geventreactor.rst
- tests/unit/io/test_eventletreactor.py
- tests/unit/io/utils.py
- benchmarks/base.py
- cassandra/io/twistedreactor.py
- tests/unit/io/test_geventreactor.py
- docs/security.rst
- tests/unit/io/gevent_utils.py
- cassandra/io/eventletreactor.py
- docs/api/index.rst
- tests/unit/io/test_twistedreactor.py
- tests/unit/io/eventlet_utils.py
- tests/integration/standard/test_connection.py
- cassandra/io/geventreactor.py
- docs/api/cassandra/io/twistedreactor.rst
e97d2dd to
69799f1
Compare
|
@coderabbitai review |
|
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cassandra/datastax/cloud/__init__.py (1)
129-129: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnable hostname verification for the metadata TLS context.
_ssl_context_from_certleavescheck_hostnamedisabled. The laterssl_optionssetting does not modify the supplied context. Setssl_context.check_hostname = Trueand add a wrong-hostname regression test.🤖 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/datastax/cloud/__init__.py` at line 129, Update the metadata TLS setup in the surrounding cloud configuration flow to set ssl_context.check_hostname = True immediately after _ssl_context_from_cert returns. Add a regression test that uses a certificate for the wrong hostname and verifies the TLS connection is rejected.Sources: Coding guidelines, MCP tools
🟡 Other comments (2)
cassandra/datastax/cloud/__init__.py-87-88 (1)
87-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve the
BadZipFilecause.Ruff B904 flags this translation. Bind the exception and raise the
ValueErrorwithfrom err.🤖 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/datastax/cloud/__init__.py` around lines 87 - 88, Update the BadZipFile handler to bind the caught exception and raise the existing ValueError using that exception as its explicit cause via from err, preserving the current message.Sources: Coding guidelines, Linters/SAST tools
CONTRIBUTING.rst-76-76 (1)
76-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when the asyncio reactor test skips.
Line 76 omits
CASS_DRIVER_NO_SKIP=1, while the corresponding CI command sets it to prevent skipped tests from passing. Add the variable so this command cannot report success without running the asyncio reactor test.Proposed fix
- EVENT_LOOP_MANAGER=asyncio uv run pytest tests/unit/io/test_asyncioreactor.py + CASS_DRIVER_NO_SKIP=1 EVENT_LOOP_MANAGER=asyncio uv run pytest tests/unit/io/test_asyncioreactor.py🤖 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 `@CONTRIBUTING.rst` at line 76, Add CASS_DRIVER_NO_SKIP=1 to the documented pytest command for test_asyncioreactor.py, alongside EVENT_LOOP_MANAGER=asyncio, so skipped asyncio reactor tests cause the command to fail.
🤖 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.
Outside diff comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Line 129: Update the metadata TLS setup in the surrounding cloud configuration
flow to set ssl_context.check_hostname = True immediately after
_ssl_context_from_cert returns. Add a regression test that uses a certificate
for the wrong hostname and verifies the TLS connection is rejected.
---
Other comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 87-88: Update the BadZipFile handler to bind the caught exception
and raise the existing ValueError using that exception as its explicit cause via
from err, preserving the current message.
In `@CONTRIBUTING.rst`:
- Line 76: Add CASS_DRIVER_NO_SKIP=1 to the documented pytest command for
test_asyncioreactor.py, alongside EVENT_LOOP_MANAGER=asyncio, so skipped asyncio
reactor tests cause the command to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: cbe95ed5-fb57-4807-89d6-edfdf362ea68
📒 Files selected for processing (3)
CONTRIBUTING.rstcassandra/datastax/cloud/__init__.pypyproject.toml
patch by Brett Abamonte; reviewed by Bret McGuire (cherry picked from commit 8b39688)
…hread_pool_executor This code path in cassandra/cluster.py was not part of the original upstream removal since it doesn't exist upstream; it's fork-specific. With eventletreactor deleted, the import always failed and the method always fell back to a plain ThreadPoolExecutor, so drop the dead branch.
a56e602 to
0601f4d
Compare
2 participants
Summary
asyncioinconn_fns, retainedis_monkey_patched()helpers used by a fork-specific libev shutdown test).EventletConnectionimport inCluster._create_thread_pool_executor— fork-specific code not present upstream, left dead after the reactor module was deleted.Test plan
python3 -m py_compileon all touched filestests/unitsuite run (no C extensions available in sandbox): 697 passed, 99 skipped (skips due to missing libev/cmurmur3/lz4 extensions, unrelated to this change), 0 failures