Skip to content

WIP #493

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

WIP #493

wants to merge 2 commits into from

Conversation

mykaul
Copy link

@mykaul mykaul commented Jun 20, 2025

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@mykaul mykaul requested a review from Copilot June 20, 2025 09:07
@mykaul mykaul marked this pull request as draft June 20, 2025 09:07
Copilot

This comment was marked as outdated.

@mykaul mykaul force-pushed the 490_remove_proto_1_2 branch 3 times, most recently from 3bcd9c1 to de02da7 Compare June 21, 2025 14:49
@mykaul
Copy link
Author

mykaul commented Jun 21, 2025

TODO: make_header_prefix() needs to be adjusted, as it used the old protocol (with stream_id being a single byte, now it's 2) . That probably explains most failures.

@mykaul mykaul force-pushed the 490_remove_proto_1_2 branch 7 times, most recently from f74ce7f to 203c576 Compare June 27, 2025 07:56
@mykaul mykaul requested a review from Copilot June 27, 2025 07:57
Copilot

This comment was marked as outdated.

@mykaul mykaul force-pushed the 490_remove_proto_1_2 branch 7 times, most recently from 3a630b3 to 86db20c Compare June 29, 2025 13:44
@mykaul mykaul requested a review from Copilot June 29, 2025 14:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes legacy support for protocol versions 1 and 2, standardizes on protocol v3+ handling throughout code, tests, and documentation, and simplifies collection serialization to always use 32-bit length prefixes.

  • Drop all v1/v2-specific branches in protocol framing, query parameters, and metadata parsing.
  • Update tests to assume protocol version 3 (or higher) as the baseline.
  • Refactor collection types to always use int32_pack/int32_unpack for lengths.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/test_parameter_binding.py Consolidated legacy BoundStatementTestV* into v3+ class
tests/unit/test_orderedmap.py Updated default protocol for OrderedMapSerializedKey
tests/unit/test_marshalling.py Switched marshalling/unmarshalling calls to proto v3+
tests/unit/test_host_connection_pool.py Removed unused get_max_requests_per_connection override
tests/unit/test_connection.py Removed legacy header logic for protocol <3
tests/unit/io/utils.py Adjusted header prefix to include MSB byte for v3+
tests/integration/.../test_cluster.py Deleted outdated tests for v1/v2 settings and decorators
tests/integration/init.py Removed obsolete protocol decorators
docs/api/cassandra/cluster.rst Removed automethod listings for min/max request APIs
cassandra/protocol.py Eliminated protocol <2 error checks and branches
cassandra/connection.py Always use v3 header struct; drop v1/v2 variants
cassandra/pool.py Removed per-connection request thresholds for v1/v2
cassandra/metadata.py Dropped keyspace update for legacy protocol versions
cassandra/cqltypes.py Always use 32-bit length prefixes; removed 16-bit paths
cassandra/cluster.py Deleted min/max requests methods and fields
cassandra/init.py Updated SUPPORTED_VERSIONS to drop v1 and v2
benchmarks/callback_full_pipeline.py Removed conditional concurrency for protocols <3
Comments suppressed due to low confidence (1)

cassandra/pool.py:1016

  • [nitpick] By removing the in_flight threshold check before spawning new connections, the pool may create connections too aggressively under load. Consider reintroducing a minimal in-flight requests threshold to prevent unnecessary connections.
            if len(self._connections) < max_conns:

@@ -180,7 +170,7 @@ class ProtocolVersion(object):
DSE private protocol v2, supported in DSE 6.0+
"""

SUPPORTED_VERSIONS = (DSE_V2, DSE_V1, V6, V5, V4, V3, V2, V1)
SUPPORTED_VERSIONS = (DSE_V2, DSE_V1, V6, V5, V4, V3)
Copy link
Preview

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SUPPORTED_VERSIONS tuple no longer includes V2 and V1, but get_supported_protocol_versions can still return versions 1 and 2. This mismatch may cause negotiation of unsupported protocols; please align both lists or enforce protocol >=3.

Copilot uses AI. Check for mistakes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense.

mykaul added 2 commits June 29, 2025 17:43
Remove most of the code related to V1, V2 protocol versions.

Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
Remove additional getters and setters that only make sense for V1/V2 protocols.

Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
@mykaul mykaul force-pushed the 490_remove_proto_1_2 branch from 86db20c to 8acc3c0 Compare June 29, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant