Skip to content
14 changes: 12 additions & 2 deletions cassandra/cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -4903,6 +4903,13 @@ def _on_speculative_execute(self):
self._start_timer()

def _make_query_plan(self):
# Clear any tablet stashed by an earlier, unrelated execution of this
# reused statement before deciding how to plan this one -- both the
# explicit-host branch below and a non-token-aware load balancing
# policy never repopulate it themselves.
if self.query is not None:
self.query._tablet = None

# set the query_plan according to the load balancing policy,
# or to the explicit host target if set
if self._host:
Expand Down Expand Up @@ -5046,11 +5053,14 @@ def _query(self, host, message=None, cb=None):
# TODO get connectTimeout from cluster settings
if self.query:
# Pass the ring token computed once for this request so the pool
# can select the shard without re-hashing the routing key.
# can select the shard without re-hashing the routing key, and
# the tablet found during query planning so the pool can skip a
# redundant lookup in the tablet map.
connection, request_id = pool.borrow_connection(
timeout=2.0, routing_key=self.query.routing_key,
keyspace=self.query.keyspace, table=self.query.table,
routing_token=self._routing_token)
routing_token=self._routing_token,
tablet=getattr(self.query, '_tablet', None))
else:
connection, request_id = pool.borrow_connection(timeout=2.0)
self._connection = connection
Expand Down
18 changes: 16 additions & 2 deletions cassandra/policies.py
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,11 @@ def make_query_plan(self, working_keyspace=None, query=None):

child = self._child_policy
if query is None or query.routing_key is None or keyspace is None:
if query is not None:
# A Statement (e.g. BoundStatement) can be rebound and
# re-executed by the caller; make sure a tablet stashed by
# an earlier, unrelated execution isn't picked up below.
query._tablet = None
for host in child.make_query_plan(keyspace, query):
yield host
return
Expand All @@ -572,10 +577,13 @@ def make_query_plan(self, working_keyspace=None, query=None):
tablet = self._cluster_metadata._tablets.get_tablet_for_key(keyspace, query.table, token)

if tablet is not None:
replicas_mapped = set(map(lambda r: r[0], tablet.replicas))
replica_dict = tablet._replica_dict
child_plan = child.make_query_plan(keyspace, query)

replicas = [host for host in child_plan if host.host_id in replicas_mapped]
replicas = [host for host in child_plan if host.host_id in replica_dict]
# Stash the tablet so that downstream shard-aware connection
# selection can reuse it instead of repeating the bisect lookup.
query._tablet = tablet

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Already addressed in this PR: cluster.py's ResponseFuture._make_query_plan() now unconditionally clears query._tablet = None at the top, before branching on self._host or calling the load balancer (added in commit a7c9dd9, after this review was posted against an earlier commit). That covers the explicit-host branch and any non-token-aware policy switch you describe, since _make_query_plan() runs once per ResponseFuture/execution and is the sole gate before the only read site (cluster.py's borrow_connection(..., tablet=getattr(self.query, '_tablet', None))). Traced all set/read sites (policies.py:563,586,631 and cluster.py:4911/5063) — no other path reads this field. Verified with a regression test exercising the explicit-host branch with a pre-set stale tablet.


# The leader concept only exists for strongly-consistent keyspaces,
# which today means exactly the keyspaces whose consistency mode is
Expand Down Expand Up @@ -615,6 +623,12 @@ def make_query_plan(self, working_keyspace=None, query=None):
break
else:
replicas = self._cluster_metadata.get_replicas(keyspace, query.routing_key)
# Clear any tablet stashed by a previous execution of this same
# query object (statements may be rebound and reused, e.g. via
# BoundStatement.bind()) so a stale tablet -- for a different
# routing key -- isn't reused for shard-aware connection
# selection below.
query._tablet = None

if self.shuffle_replicas and not query.is_lwt() and not ConsistencyLevel.is_serial(query.consistency_level):
shuffle(replicas)
Expand Down
22 changes: 11 additions & 11 deletions cassandra/pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ def __init__(self, host, host_distance, session):

log.debug("Finished initializing connection for host %s", self.host)

def _get_connection_for_routing_key(self, routing_key=None, keyspace=None, table=None, routing_token=None):
def _get_connection_for_routing_key(self, routing_key=None, keyspace=None, table=None, routing_token=None, tablet=None):
if self.is_shutdown:
raise ConnectionException(
"Pool for %s is shutdown" % (self.host,), self.host)
Expand All @@ -463,19 +463,19 @@ def _get_connection_for_routing_key(self, routing_key=None, keyspace=None, table
if t is None and metadata.token_map is not None and metadata.can_support_partitioner():
t = metadata.token_map.token_class.from_key(routing_key)
if t is not None and self.supports_tablet_routing and table is not None:
if keyspace is None:
keyspace = self._keyspace
# Reuse the tablet found during query planning when available,
# avoiding a redundant bisect lookup in the tablet map.
if tablet is None:
if keyspace is None:
keyspace = self._keyspace

tablet = self._session.cluster.metadata._tablets.get_tablet_for_key(keyspace, table, t)
tablet = self._session.cluster.metadata._tablets.get_tablet_for_key(keyspace, table, t)

# In both V1 and V2 the request is sent to this host, so we pick
# the shard that this host owns for the tablet. Leader-aware host
# selection (V2) happens earlier, in the load balancing policy.
if tablet is not None:
for replica in tablet.replicas:
if replica[0] == self.host.host_id:
shard_id = replica[1]
break
shard_id = tablet._replica_dict.get(self.host.host_id)

if shard_id is None and t is not None:
shard_id = self.host.sharding_info.shard_id_from_token(t.value)
Expand Down Expand Up @@ -518,15 +518,15 @@ def _get_connection_for_routing_key(self, routing_key=None, keyspace=None, table
return random.choice(active_connections)
return random.choice(list(self._connections.values()))

def borrow_connection(self, timeout, routing_key=None, keyspace=None, table=None, routing_token=None):
conn = self._get_connection_for_routing_key(routing_key, keyspace, table, routing_token)
def borrow_connection(self, timeout, routing_key=None, keyspace=None, table=None, routing_token=None, tablet=None):
conn = self._get_connection_for_routing_key(routing_key, keyspace, table, routing_token, tablet)
start = time.time()
remaining = timeout
last_retry = False
while True:
if conn.is_closed:
# The connection might have been closed in the meantime - if so, try again
conn = self._get_connection_for_routing_key(routing_key, keyspace, table, routing_token)
conn = self._get_connection_for_routing_key(routing_key, keyspace, table, routing_token, tablet)
with conn.lock:
if (not conn.is_closed or last_retry) and conn.in_flight < conn.max_request_id:
# On last retry we ignore connection status, since it is better to return closed connection than
Expand Down
126 changes: 75 additions & 51 deletions cassandra/tablets.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
from bisect import bisect_left
from operator import attrgetter
from random import getrandbits
from threading import Lock
from typing import Optional
from uuid import UUID

# C-accelerated attrgetter avoids per-call lambda allocation overhead
_get_first_token = attrgetter("first_token")
_get_last_token = attrgetter("last_token")


def choose_tablet_version_block(tablet_version: int) -> int:
"""
Expand Down Expand Up @@ -42,39 +37,39 @@ class Tablet(object):
It stores information about each replica, its host and shard,
and the token interval in the format (first_token, last_token].
"""
first_token = 0
last_token = 0
replicas = None
# uint64 hash; None means unknown -- a cold start, or a tablet learned over
# TABLETS_ROUTING_V1, which does not report a version.
tablet_version = None
__slots__ = ('first_token', 'last_token', 'replicas', 'tablet_version', '_replica_dict')

def __init__(self, first_token=0, last_token=0, replicas=None, tablet_version=None):
self.first_token = first_token
self.last_token = last_token
self.replicas = replicas
# Materialize once: `replicas` may be a one-shot iterator, and both
# the tuple and the lookup dict must come from the same iteration.
self.replicas = tuple(replicas) if replicas is not None else None
self._replica_dict = {r[0]: r[1] for r in self.replicas} if self.replicas else {}
self.tablet_version = tablet_version

def __str__(self):
return "<Tablet: first_token=%s last_token=%s replicas=%s tablet_version=%s>" \
% (self.first_token, self.last_token, self.replicas, self.tablet_version)
__repr__ = __str__

@staticmethod
def _is_valid_tablet(replicas):
return replicas is not None and len(replicas) != 0

@staticmethod
def from_row(first_token, last_token, replicas, tablet_version=None):
if Tablet._is_valid_tablet(replicas):
if tablet_version is not None:
# tablet_version is an unsigned 64-bit value, but it is
# deserialized from the wire as a signed LongType; normalize it
# back to unsigned so it matches the server's representation.
tablet_version &= 0xFFFFFFFFFFFFFFFF
tablet = Tablet(first_token, last_token, replicas, tablet_version)
return tablet
return None
# Materialize once: `replicas` may be a one-shot iterator (e.g. a
# generator), and a plain `if not replicas` truthiness check would
# always be False for such an object even when it yields nothing,
# since iterators have no __len__/__bool__ and are always truthy.
replicas_tuple = tuple(replicas) if replicas is not None else ()
if not replicas_tuple:
return None
if tablet_version is not None:
# tablet_version is an unsigned 64-bit value, but it is
# deserialized from the wire as a signed LongType; normalize it
# back to unsigned so it matches the server's representation.
tablet_version &= 0xFFFFFFFFFFFFFFFF
return Tablet(first_token, last_token, replicas_tuple, tablet_version)

@property
def leader(self) -> Optional[UUID]:
Expand Down Expand Up @@ -104,37 +99,54 @@ def leader(self) -> Optional[UUID]:
return self.replicas[0][0]

def replica_contains_host_id(self, uuid: UUID) -> bool:
for replica in self.replicas:
if replica[0] == uuid:
return True
return False
return uuid in self._replica_dict

def get_replica_shard_id(self, uuid: UUID) -> Optional[int]:
return self._replica_dict.get(uuid)

class Tablets(object):
_lock = None
_tablets = {}

class Tablets(object):
def __init__(self, tablets):
self._tablets = tablets
# NOTE: these are intentionally instance attributes only (not class
# attributes) to avoid mutable class-level dicts being shared across
# instances, e.g. if a future alternative constructor were to bypass
# __init__.
self._lock = Lock()
self._tablets = tablets
# Build parallel token index lists from any pre-populated data
# (keyspace, table) -> list[int] for both _first_tokens/_last_tokens
self._first_tokens = {
key: [t.first_token for t in tlist]
for key, tlist in tablets.items()
}
self._last_tokens = {
key: [t.last_token for t in tlist]
for key, tlist in tablets.items()
}

def table_has_tablets(self, keyspace, table) -> bool:
return bool(self._tablets.get((keyspace, table), []))

def get_tablet_for_key(self, keyspace, table, t):
tablet = self._tablets.get((keyspace, table), [])
if not tablet:
key = (keyspace, table)
with self._lock:
last_tokens = self._last_tokens.get(key)
if not last_tokens:
return None

token_value = t.value
id = bisect_left(last_tokens, token_value)
if id < len(last_tokens) and token_value > self._first_tokens[key][id]:
return self._tablets[key][id]
return None

id = bisect_left(tablet, t.value, key=_get_last_token)
if id < len(tablet) and t.value > tablet[id].first_token:
return tablet[id]
return None

def drop_tablets(self, keyspace: str, table: Optional[str] = None):
with self._lock:
if table is not None:
self._tablets.pop((keyspace, table), None)
key = (keyspace, table)
self._tablets.pop(key, None)
self._first_tokens.pop(key, None)
self._last_tokens.pop(key, None)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return

to_be_deleted = []
Expand All @@ -144,36 +156,48 @@ def drop_tablets(self, keyspace: str, table: Optional[str] = None):

for key in to_be_deleted:
del self._tablets[key]
self._first_tokens.pop(key, None)
self._last_tokens.pop(key, None)

def drop_tablets_by_host_id(self, host_id: Optional[UUID]):
if host_id is None:
return
with self._lock:
for key, tablets in self._tablets.items():
to_be_deleted = []
for tablet_id, tablet in enumerate(tablets):
if tablet.replica_contains_host_id(host_id):
to_be_deleted.append(tablet_id)

for tablet_id in reversed(to_be_deleted):
tablets.pop(tablet_id)
# Filter in one pass instead of popping one-by-one (O(n) vs O(k*n))
keep = [i for i, t in enumerate(tablets)
if not t.replica_contains_host_id(host_id)]
if len(keep) == len(tablets):
continue # nothing to drop
self._tablets[key] = [tablets[i] for i in keep]
first = self._first_tokens[key]
last = self._last_tokens[key]
self._first_tokens[key] = [first[i] for i in keep]
self._last_tokens[key] = [last[i] for i in keep]

def add_tablet(self, keyspace, table, tablet):
with self._lock:
tablets_for_table = self._tablets.setdefault((keyspace, table), [])
key = (keyspace, table)
tablets_for_table = self._tablets.setdefault(key, [])
first_tokens = self._first_tokens.setdefault(key, [])
last_tokens = self._last_tokens.setdefault(key, [])

# find first overlapping range
start = bisect_left(tablets_for_table, tablet.first_token, key=_get_first_token)
if start > 0 and tablets_for_table[start - 1].last_token > tablet.first_token:
start = bisect_left(first_tokens, tablet.first_token)
if start > 0 and last_tokens[start - 1] > tablet.first_token:
start = start - 1

# find last overlapping range
end = bisect_left(tablets_for_table, tablet.last_token, key=_get_last_token)
if end < len(tablets_for_table) and tablets_for_table[end].first_token >= tablet.last_token:
end = bisect_left(last_tokens, tablet.last_token)
if end < len(last_tokens) and first_tokens[end] >= tablet.last_token:
end = end - 1

if start <= end:
del tablets_for_table[start:end + 1]
del first_tokens[start:end + 1]
del last_tokens[start:end + 1]

tablets_for_table.insert(start, tablet)
first_tokens.insert(start, tablet.first_token)
last_tokens.insert(start, tablet.last_token)

6 changes: 5 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,12 @@ test-extras = ["compress-lz4"]
# * test_deserialize_date_range_month is disabled upstream (PYTHON-912).
# PyPy uses the pp* override below. The Linux CPython reactor command runs with
# CASS_DRIVER_NO_SKIP=1 so unexpected skips fail loudly.
# test_datetype compares serialize() of a datetime vs. a raw float timestamp
# taken microseconds apart; it can flake across a millisecond rounding boundary
# on slower/emulated runners (e.g. linux-aarch64), same class of issue as the
# macOS timing exclusions below.
test-command = [
"CASS_DRIVER_NO_SKIP=1 pytest --import-mode=append {package}/tests/unit -v --ignore={package}/tests/unit/column_encryption --ignore={package}/tests/unit/io/test_asyncioreactor.py --ignore={package}/tests/unit/io/test_asyncorereactor.py -k 'not test_deserialize_date_range_month'",
"CASS_DRIVER_NO_SKIP=1 pytest --import-mode=append {package}/tests/unit -v --ignore={package}/tests/unit/column_encryption --ignore={package}/tests/unit/io/test_asyncioreactor.py --ignore={package}/tests/unit/io/test_asyncorereactor.py -k 'not (test_deserialize_date_range_month or test_datetype)'",
"EVENT_LOOP_MANAGER=asyncio CASS_DRIVER_NO_SKIP=1 pytest --import-mode=append {package}/tests/unit/io/test_asyncioreactor.py -v",
]

Expand Down
Loading
Loading