Conversation
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
Thanks for the PR @Ya-Sabyr!
Some of these changes have been implemented elsewhere in other PRs; those should probably be reverted from this PR. There are also some questions around some of the SSL changes that I think we can talk about more if you'd like.
| log = logging.getLogger(__name__) | ||
|
|
||
| conn_fns = (_try_gevent_import, _try_eventlet_import, _try_libev_import, _try_asyncore_import) | ||
| conn_fns = (_try_gevent_import, _try_eventlet_import, _try_libev_import, _try_asyncio_import, _try_asyncore_import) |
There was a problem hiding this comment.
We definitely don't want to do this until the asyncio event loop is stable. We describe the asyncio reactor as "experimental" with good reason so I don't want to include this reactor in the default sequence until we're able to get it stabilized. The current plan is to work on that for the next release (3.31.0). I'd be okay with adding it to the default reactors at that point but we need to get there first.
| "Programming Language :: Python :: 3.11", | ||
| "Programming Language :: Python :: 3.12", | ||
| "Programming Language :: Python :: 3.13", | ||
| "Programming Language :: Python :: 3.14", |
There was a problem hiding this comment.
We missed this in the original work on CASSPYTHON-3. This was subsequently addressed in the PR for CASSPYTHON-12.
Note that since only Python 3.10 through 3.14 will be non-EOL at time of release you also need to remove Python 3.9 from the list of classifiers here.
| Testing Multiple Python Versions | ||
| -------------------------------- | ||
| Use tox to test all of Python 3.9 through 3.13 and pypy:: | ||
| Use tox to test all of Python 3.9 through 3.14 and pypy:: |
There was a problem hiding this comment.
This is being addressed in the 3.30.0 changelog and documentation PR
| DataStax Enterprise (4.7+) using exclusively Cassandra's binary protocol and Cassandra Query Language v3. | ||
|
|
||
| The driver supports Python 3.9 through 3.13. | ||
| The driver supports Python 3.9 through 3.14. |
There was a problem hiding this comment.
This is being addressed in the 3.30.0 changelog and documentation PR
| @@ -1,5 +1,5 @@ | |||
| [tox] | |||
| envlist = py{39,310,311,312,313},pypy | |||
| envlist = py{39,310,311,312,313,314},pypy | |||
There was a problem hiding this comment.
This is being addressed in the 3.30.0 changelog and documentation PR
| session = cluster.connect(wait_for_all_pools=True) | ||
| self.assertGreater(len(cluster.metadata.all_hosts()), 1, "We only have one host connected at this point") | ||
| if len(cluster.metadata.all_hosts()) <= 1: | ||
| raise unittest.SkipTest("This test requires multiple connected hosts") |
There was a problem hiding this comment.
I don't think we want to silently skip tests here if we're not able to connect to the nodes. These tests are all configured to use a three-node ccm cluster and since we also wait on all connection pools to complete before returning (note the "wait_for_all_pools" arg to the connect() call) we should always have at least three connections here. If we don't something's gone quite wrong (or our test setup is bogus) so we should provide some feedback to the test runner to indicate that fact rather than silently skipping this test.
As an example: I ran this test locally and saw it pass right away. It's also been passing in all our Jenkins runs. If you're seeing local failures @Ya-Sabyr it's probably something that should be investigated more completely rather than skipped.
| def _ssl_context_from_cert(ca_cert_location, cert_location, key_location): | ||
| ssl_context = SSLContext(PROTOCOL_TLS) | ||
| protocol = getattr(ssl, "PROTOCOL_TLS_CLIENT", ssl.PROTOCOL_TLS) | ||
| ssl_context = SSLContext(protocol) |
There was a problem hiding this comment.
Can you speak more about what this was intended to do @Ya-Sabyr? PROTOCOL_TLS_CLIENT has been included in the ssl module since 3.6 so this will always succeed, at least for versions of Python we explicitly support. This means that by default we will always use PROTOCOL_TLS_CLIENT for SSL ops against Astra which may offer some benefit (although that isn't clear to me). We could talk about that but I'm not sure why we wouldn't just use this instead:
ssl_context = SSLContext(PROTOCOL_TLS_CLIENT)
| ssl_context.load_verify_locations(ca_cert_location) | ||
| ssl_context.verify_mode = CERT_REQUIRED | ||
| if hasattr(ssl_context, "check_hostname"): | ||
| ssl_context.check_hostname = True |
There was a problem hiding this comment.
A similar question here. check_hostname has been on SSLContext since Python 3.4 so the hasattr() check here will always return true (at least for modern Pythons). Implication there is that we'll always set check_hostname to True, which (a) may be a good idea anyway but (b) is definitely a change that should be considered by itself.
Perhaps more directly relevant: PROTOCOL_TLS_CLIENT enables this by default so this is actually redundant given the change above.
2 participants
No description provided.