-
Notifications
You must be signed in to change notification settings - Fork 49
Remove protocol versions >5 from MAX_SUPPORTED #492
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
Conversation
11a07bc
to
7042125
Compare
Need to handle this failure:
|
In #493 I've suggested (and implemented) removing protocols v1, v2. Here we need to decide if we wish to remove the higher versions. |
I'm fine with removing DSE protocols, and all DSE functionality in general. |
e1ef8db
to
ececd69
Compare
There was a problem hiding this 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 restricts the driver to native protocol versions 3–5, makes V5 the default, and cleans up all DSE- and beta-protocol-specific paging logic and tests.
- Limit supported protocol versions to V3–V5 and set default to V5.
- Remove DSE/backpressured continuous-paging “next_pages” and queue-size behavior.
- Update and prune unit/integration tests to drop references to removed protocols and adapt compression negotiation.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
cassandra/init.py | Trim SUPPORTED_VERSIONS to (V5, V4, V3) and update docstring |
cassandra/cluster.py | Change protocol_version default to V5; remove DSE‐paging support |
cassandra/protocol.py | Drop writing of queue size and backpressure support |
tests/unit/test_protocol.py | Simplify continuous-paging tests to only check unsupported cases |
tests/unit/test_connection.py | Adapt compression negotiation to protocol V5 behavior |
tests/integration/init.py | Update default/supported/unsupported protocol lists |
tests/integration/* | Prune DSE‐ and protocol-6 tests and related decorators |
Comments suppressed due to low confidence (2)
cassandra/protocol.py:617
- Since the code for writing
max_queue_size
has been removed, consider adding a unit test to verify that under protocol V5 onlymax_pages
andmax_pages_per_second
are written and no queue-size field is emitted.
write_int(f, paging_options.max_pages_per_second)
cassandra/init.py:175
- [nitpick] The docstring is now misleading; it should be updated to accurately describe the current supported versions (V3–V5) rather than referring to “future v5 support.”
A tuple of all supported protocol versions for ScyllaDB, including future v5 support.
Ugh. Wrong rebase. |
TODO: need to remove this continuous paging support. |
Done, to some extent. I think it's reasonable. |
Flakiness?
|
Rebased, passed CI. I think it's OK now. |
If someone can review this, that'd be great. Not urgent. |
Same request, still not urgent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some problem with how these two patches are split. The first patch claims it doesn't remove code, and yet it does remove a bunch of "continuous paging" and "graph paging" (whatever the heck that is) from the code - which later seems to be the focus of the second patch. Can you perhaps re-split the patches to better fit the descriptions?
Why did you want to remove the continuous paging stuff? What have we got to lose by having in the driver some DSE-only code? I know we don't recommend anyone using it, but if in the future we'll ever want to merge back with the upstream driver it will be easier if we don't change random pieces or the driver.
Or have we already given up the hope of merging from or to the upstream?
b1fb07d
to
6ecf818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this PR, but it should be reviewed (and merged) by the drivers developers.
@scylladb/python-driver-maint - please review (not urgent). |
I'm tempted to remove some more code, such as making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one question though
The main goal of this patch is to have the driver start negotiating protocol versions at 5, and not begin by attempting DSE-specific protocols or protocol version 6, which Scylla doesn't support and cause protocol errors which only wastes time in the beginning of each connection. But then, since the driver won't even try DSE-specific protocols, we don't need to support them and can begin to remove DSE-specific code. This patch only removes some obvious things, but there's still a bunch of DSE-specific code that hasn't been removed yet. Refs: scylladb#244 Signed-off-by: Yaniv Kaul <[email protected]>
I'm not removing it entirely from the code, but ignoring it where it matters, so it's essentially useless. I think it's a reasonable balance between breaking the API of some classes and removal of the feature. Signed-off-by: Yaniv Kaul <[email protected]>
6ecf818
to
d123401
Compare
Per review comments, reverted the changes to test, but changed it to explicitly use v4 of the protocol.
d123401
to
af169f2
Compare
Without removing associated code yet, this patch just ensure we begin negotiating protocol version 5, then fallback to 4, reducing the amount of protocol errors that the Scylla server sends when it sees the unsupported versions.
Refs: #244
Perhaps we should add documentation to this?
Pre-review checklist
./docs/source/
.Fixes:
annotations to PR description.