-
Notifications
You must be signed in to change notification settings - Fork 56
fix: detect ScyllaDB via SUPPORTED protocol extensions, not shard count #910
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,21 +17,43 @@ class ProtocolFeatures(object): | |
| sharding_info = None | ||
| tablets_routing_v1 = False | ||
| lwt_info = None | ||
| is_scylla = False | ||
|
|
||
| def __init__(self, rate_limit_error=None, shard_id=0, sharding_info=None, tablets_routing_v1=False, lwt_info=None): | ||
| def __init__(self, rate_limit_error=None, shard_id=0, sharding_info=None, tablets_routing_v1=False, lwt_info=None, is_scylla=False): | ||
| self.rate_limit_error = rate_limit_error | ||
| self.shard_id = shard_id | ||
| self.sharding_info = sharding_info | ||
| self.tablets_routing_v1 = tablets_routing_v1 | ||
| self.lwt_info = lwt_info | ||
| self.is_scylla = is_scylla | ||
|
|
||
| @staticmethod | ||
| def parse_from_supported(supported): | ||
| rate_limit_error = ProtocolFeatures.maybe_parse_rate_limit_error(supported) | ||
| shard_id, sharding_info = ProtocolFeatures.parse_sharding_info(supported) | ||
| tablets_routing_v1 = ProtocolFeatures.parse_tablets_info(supported) | ||
| lwt_info = ProtocolFeatures.parse_lwt_info(supported) | ||
| return ProtocolFeatures(rate_limit_error, shard_id, sharding_info, tablets_routing_v1, lwt_info) | ||
| is_scylla = ProtocolFeatures.detect_scylla(supported, sharding_info) | ||
| return ProtocolFeatures(rate_limit_error, shard_id, sharding_info, tablets_routing_v1, lwt_info, is_scylla) | ||
|
|
||
| @staticmethod | ||
| def detect_scylla(supported, sharding_info): | ||
| """Detect ScyllaDB from SUPPORTED extensions, independent of shard awareness. | ||
|
|
||
| ScyllaDB is identified by the presence of any known Scylla-specific | ||
| extension key in the SUPPORTED response. Checking only shard-related | ||
| fields (SCYLLA_NR_SHARDS, etc.) is insufficient because those are | ||
| absent when shard-awareness is disabled on the server side | ||
| (allow_shard_aware_drivers: false), which would cause the driver to | ||
| misidentify a ScyllaDB cluster as Cassandra and, for example, try | ||
| to query the peers_v2 table that ScyllaDB does not support. | ||
| """ | ||
| return ( | ||
| LWT_ADD_METADATA_MARK in supported | ||
| or RATE_LIMIT_ERROR_EXTENSION in supported | ||
| or TABLETS_ROUTING_V1 in supported | ||
| or sharding_info is not None | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def maybe_parse_rate_limit_error(supported): | ||
|
|
@@ -73,8 +95,12 @@ def parse_sharding_info(options): | |
| sharding_algorithm == "biased-token-round-robin" or sharding_ignore_msb): | ||
| return 0, None | ||
|
|
||
| return int(shard_id), _ShardingInfo(shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, | ||
| shard_aware_port, shard_aware_port_ssl) | ||
| # SCYLLA_SHARD may be absent even when other shard fields are present | ||
| # (e.g. the connection landed on shard 0 and the server omits the field). | ||
| # Default to 0 to avoid int(None) crash. | ||
| resolved_shard_id = int(shard_id) if shard_id is not None else 0 | ||
| return resolved_shard_id, _ShardingInfo(shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, | ||
| shard_aware_port, shard_aware_port_ssl) | ||
|
Comment on lines
+101
to
+103
Comment on lines
+98
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can SCYLLA_SHARD be absent, as this comment indicates?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the whole issue that started this thread - if Scylla server-side disables sharding, it doesn't advertise it whatsoever. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is not what I'm asking.
Afaik disabling sharding on scylla-side will make it not send ANY sharding-related values in SUPPORTED, which makes this comment false. See the code fragment I linked - all sharding related values are guarded by
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah. OK. |
||
|
|
||
|
|
||
| @staticmethod | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,10 +21,10 @@ | |
| class _ShardingInfo(object): | ||
|
|
||
| def __init__(self, shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, shard_aware_port, shard_aware_port_ssl): | ||
| self.shards_count = int(shards_count) | ||
| self.shards_count = int(shards_count) if shards_count else 0 | ||
| self.partitioner = partitioner | ||
| self.sharding_algorithm = sharding_algorithm | ||
| self.sharding_ignore_msb = int(sharding_ignore_msb) | ||
| self.sharding_ignore_msb = int(sharding_ignore_msb) if sharding_ignore_msb else 0 | ||
| self.shard_aware_port = int(shard_aware_port) if shard_aware_port else None | ||
| self.shard_aware_port_ssl = int(shard_aware_port_ssl) if shard_aware_port_ssl else None | ||
|
Comment on lines
22
to
29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is possible for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So do you want me to change the code to check for everything works with or without _ShardingInfo? I thought it might be a slightly bigger change. I can look into it. |
||
|
|
||
|
|
||
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.
ProtocolFeaturesalways haveis_scyllaattribute after your changes, why do you needgetattrhere?Also, is it possible for connection to not have
features?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.
It was mostly a copy-paste from the original code, IIRC. I'll look into it.
(I'm also unsure - what if self.connection is null?)