fix(lw-deletions): Query system.parts on storage node instead of query node#7772
fix(lw-deletions): Query system.parts on storage node instead of query node#7772
Conversation
…y node Query nodes only have distributed tables (_dist), so system.parts returns no rows for _local tables, causing partition-split deletes to always fall back to un-split. Connect to a storage node via get_local_nodes() instead. Also addresses PR #7766 review feedback: - Move Redis client initialization to __init__ - Use relative week offset for partition metrics to reduce tag cardinality Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In the distributed test suite, get_local_nodes() queries system.clusters which isn't available. Mock it to return a dummy node alongside the existing get_node_connection mock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| assert isinstance(schema, TableSchema) | ||
| partition_format = schema.get_partition_format() | ||
| assert partition_format is not None | ||
| parts = [decode_part_str(part, partition_format) for (part,) in response.results] |
There was a problem hiding this comment.
Duplicated get_active_partitions logic instead of reusing existing function
Medium Severity
The new inline code in _get_partition_dates (the query, schema lookup, decode_part_str call) is an exact copy of get_active_partitions from snuba/cleanup.py. Since get_active_partitions accepts a ClickhousePool as its first parameter and get_node_connection returns a ClickhousePool, the fix only needed to change the connection passed to the existing function — not duplicate all 12+ lines. Maintaining two identical copies risks divergent bug fixes.
| member = f"{table}:{partition_date}" | ||
| if redis_client.sismember(tracking_key, member): | ||
| days_delta = (datetime.strptime(partition_date, "%Y-%m-%d") - datetime.now()).days | ||
| partition_week = str(days_delta // 7) |
There was a problem hiding this comment.
Week offset miscalculated due to time-of-day component
Low Severity
datetime.strptime(partition_date, "%Y-%m-%d") produces midnight, while datetime.now() includes the current time. The .days attribute of the resulting timedelta is systematically one less than the actual calendar-day difference (e.g., today's partition yields .days = -1 instead of 0). This shifts all partition_week tags by roughly one day, so a partition from today gets week "-1" rather than the expected "0".
| member = f"{table}:{partition_date}" | ||
| if redis_client.sismember(tracking_key, member): | ||
| days_delta = (datetime.strptime(partition_date, "%Y-%m-%d") - datetime.now()).days | ||
| partition_week = str(days_delta // 7) |
There was a problem hiding this comment.
I was thinking this would be the week number in the year like datetime.now().isocalendar().week , not sure if that makes more or less sense than what you have here
There was a problem hiding this comment.
that's actually what the AI did first, but I think it makes more sense to normalize it the way we do weeks_ago in some existing metrics
2 participants
_get_partition_dateswas queryingsystem.partsviacluster.get_query_connection(), which connects to a query node. In ourcluster topology, query nodes only have distributed tables (
_dist), notthe local tables (
_local) listed inDeletionSettings.tables. This meantsystem.partsalways returned zero rows, causing every partition-splitdelete to fall back to un-split.
The fix connects to a storage node via
cluster.get_local_nodes()[0]instead, where the local tables and their
system.partsmetadata actuallylive. This mirrors how the optimize CLI handles the same problem (it
requires an explicit
--clickhouse-hostpointing at a storage node).Also incorporates review feedback from #7766:
__init__instead of on every_execute_delete_by_partitioncall-4,0,+2)instead of full dates to reduce cardinality
Fixes the partition-split fallback observed in production.