absurdfarce commented on code in PR #1281: URL: https://github.com/apache/cassandra-python-driver/pull/1281#discussion_r2977635083
########## README.rst: ########## @@ -15,7 +15,7 @@ Apache Cassandra Python Driver A modern, `feature-rich <https://github.com/datastax/python-driver#features>`_ and highly-tunable Python client library for Apache Cassandra (2.1+) and 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. Review Comment: This is being addressed in the 3.30.0 changelog and documentation [PR](https://github.com/apache/cassandra-python-driver/pull/1282) ########## pyproject.toml: ########## @@ -24,6 +24,7 @@ classifiers = [ "Programming Language :: Python :: 3.11", "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", + "Programming Language :: Python :: 3.14", Review Comment: We missed this in the original work on CASSPYTHON-3. This was subsequently addressed in the [PR](https://github.com/apache/cassandra-python-driver/pull/1279) 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. ########## README-dev.rst: ########## @@ -96,7 +96,7 @@ it with the ``PROTOCOL_VERSION`` environment variable:: 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:: Review Comment: This is being addressed in the 3.30.0 changelog and documentation [PR](https://github.com/apache/cassandra-python-driver/pull/1282) ########## tests/integration/standard/test_cluster.py: ########## @@ -996,7 +996,8 @@ def test_clone_shared_lbp(self): exec_profiles = {'rr1': rr1} with TestCluster(execution_profiles=exec_profiles) as cluster: 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") Review 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](https://github.com/apache/cassandra-python-driver/blob/3.29.3/tests/integration/standard/test_cluster.py#L51) 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. ########## cassandra/datastax/cloud/__init__.py: ########## @@ -171,9 +172,12 @@ def parse_metadata_info(config, http_data): 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) 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 Review 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. ########## cassandra/cluster.py: ########## @@ -168,7 +175,7 @@ def _connection_reduce_fn(val,import_fn): 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) Review Comment: We definitely don't want to do this until the asyncio event loop is stable. We [describe](https://github.com/apache/cassandra-python-driver/blob/3.29.3/cassandra/io/asyncioreactor.py#L69-L75) 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](https://docs.google.com/document/d/1BLKmOatbF3rr4nUwssWy98bWGgzfQSPjbKM_G0LDd6w/edit?usp=sharing) 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. ########## cassandra/datastax/cloud/__init__.py: ########## @@ -171,9 +172,12 @@ def parse_metadata_info(config, http_data): 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) Review 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) ``` ########## tox.ini: ########## @@ -1,5 +1,5 @@ [tox] -envlist = py{39,310,311,312,313},pypy +envlist = py{39,310,311,312,313,314},pypy Review Comment: This is being addressed in the 3.30.0 changelog and documentation [PR](https://github.com/apache/cassandra-python-driver/pull/1282) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
