Split space usage by replicas #425
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRefactors object storage space usage accounting to store and return usage split by replica, adjusting table schemas, fill queries, and tests accordingly. Sequence diagram for collecting and filling replica-split space usagesequenceDiagram
participant Caller
participant _collect_space_usage
participant _get_fill_space_usage_query
participant ClickHouse
Caller->>_collect_space_usage: invoke with ctx, tables, config
_collect_space_usage->>_get_fill_space_usage_query: build space usage fill queries
_get_fill_space_usage_query-->>_collect_space_usage: list[Query] space_usage_queries
loop for each Query in space_usage_queries
_collect_space_usage->>ClickHouse: execute_query(ctx, query, settings)
ClickHouse-->>_collect_space_usage: execution result (ignored)
end
_collect_space_usage->>ClickHouse: TRUNCATE old space_usage_table
ClickHouse-->>_collect_space_usage: ok
_collect_space_usage->>ClickHouse: RENAME space_usage_table_new TO space_usage_table
ClickHouse-->>_collect_space_usage: ok
_collect_space_usage-->>Caller: return
Sequence diagram for reading replica-split space usagesequenceDiagram
participant Caller
participant get_object_storage_space_usage
participant ClickHouse
Caller->>get_object_storage_space_usage: ctx, space_usage_table, scope, cluster_name
get_object_storage_space_usage->>ClickHouse: SELECT toString(replica) replicas, state, size FROM space_usage_table
ClickHouse-->>get_object_storage_space_usage: JSON rows[{replicas, state, size}]
loop for each row in rows
get_object_storage_space_usage->>get_object_storage_space_usage: ensure result[replicas] exists
get_object_storage_space_usage->>get_object_storage_space_usage: result[replicas][state] = size
end
get_object_storage_space_usage-->>Caller: dict[replica][state] -> size
ER diagram for updated object storage space usage tableserDiagram
local_blobs_table {
String replica
String obj_path
String state
UInt64 obj_size
UInt32 ref_count
}
orphaned_blobs_table {
String obj_path
UInt64 obj_size
}
space_usage_table {
Array_String replica
String state
UInt64 size
}
local_blobs_table ||--o{ space_usage_table : aggregates_into
orphaned_blobs_table ||--o{ space_usage_table : contributes_orphaned_size
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
_get_fill_space_usage_querythegroupArraySorted(10)(replica)limit is a silent constraint on the number of replicas; consider either removing the hard-coded10or making it a named constant with a short comment so the behavior is explicit and maintainable. - The two branches of
_get_fill_space_usage_query(25.3+ vs older) now duplicate a fair amount of logic for computingactive,unique_frozen, andunique_detached; it may be worth factoring out the shared subqueries/structure to reduce the chance of the branches drifting out of sync. - In
_create_space_usage_table, thectxline has a stale inline comment# , unique_frozen UInt64, unique_detached UInt64, orphaned UInt64)which no longer matches the schema; consider cleaning this up to avoid confusion about the current table layout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_get_fill_space_usage_query` the `groupArraySorted(10)(replica)` limit is a silent constraint on the number of replicas; consider either removing the hard-coded `10` or making it a named constant with a short comment so the behavior is explicit and maintainable.
- The two branches of `_get_fill_space_usage_query` (25.3+ vs older) now duplicate a fair amount of logic for computing `active`, `unique_frozen`, and `unique_detached`; it may be worth factoring out the shared subqueries/structure to reduce the chance of the branches drifting out of sync.
- In `_create_space_usage_table`, the `ctx` line has a stale inline comment `# , unique_frozen UInt64, unique_detached UInt64, orphaned UInt64)` which no longer matches the schema; consider cleaning this up to avoid confusion about the current table layout.
## Individual Comments
### Comment 1
<location> `ch_tools/common/commands/object_storage.py:1205` </location>
<code_context>
+ UNION ALL
+ SELECT replica, 'unique_detached', size FROM unique_detached
+ UNION ALL
+ SELECT ['all_replicas'], 'orphaned', toFloat64(sum(obj_size)) FROM {orphaned_blobs_table}
"""
+ ]
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting orphaned size to Float64 conflicts with the UInt64 column type and may introduce precision or implicit-cast issues.
`size` in `{space_usage_table_new}` is `UInt64`, but this INSERT uses `toFloat64(sum(obj_size))`, forcing an implicit cast and potential precision/plan issues. Since `obj_size` is already `UInt64`, keep `sum(obj_size)` as `UInt64` and remove `toFloat64` here (and in the non-25.3 branch).
</issue_to_address>
### Comment 2
<location> `ch_tools/common/commands/object_storage.py:1160` </location>
<code_context>
+ WITH
+ aggregated_blobs AS (
+ SELECT
+ groupArraySorted(10)(replica) as replica,
+ obj_path,
+ state,
</code_context>
<issue_to_address>
**issue (bug_risk):** Limiting `groupArraySorted` to 10 replicas can silently undercount space usage on clusters with more replicas.
Because only the first 10 replicas per `obj_path`+`state` are kept in the `replica` array, any additional replicas are dropped and never appear in the downstream per-replica aggregation. That will under-report usage on clusters with >10 replicas. If you need all replicas to be counted, remove the limit (`groupArraySorted(replica)`) or use another approach that preserves the full replica set.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| UNION ALL | ||
| SELECT replica, 'unique_detached', size FROM unique_detached | ||
| UNION ALL | ||
| SELECT ['all_replicas'], 'orphaned', toFloat64(sum(obj_size)) FROM {orphaned_blobs_table} |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Casting orphaned size to Float64 conflicts with the UInt64 column type and may introduce precision or implicit-cast issues.
size in {space_usage_table_new} is UInt64, but this INSERT uses toFloat64(sum(obj_size)), forcing an implicit cast and potential precision/plan issues. Since obj_size is already UInt64, keep sum(obj_size) as UInt64 and remove toFloat64 here (and in the non-25.3 branch).
| WITH | ||
| aggregated_blobs AS ( | ||
| SELECT | ||
| groupArraySorted(10)(replica) as replica, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Limiting groupArraySorted to 10 replicas can silently undercount space usage on clusters with more replicas.
Because only the first 10 replicas per obj_path+state are kept in the replica array, any additional replicas are dropped and never appear in the downstream per-replica aggregation. That will under-report usage on clusters with >10 replicas. If you need all replicas to be counted, remove the limit (groupArraySorted(replica)) or use another approach that preserves the full replica set.
aalexfvk
reviewed
Dec 19, 2025
| """ | ||
| batch_values = ",".join( | ||
| f"('{item.key if item.key_is_full else os.path.join(disk_conf.prefix, item.key)}','shadow',{item.size},1)" | ||
| f"('missing_backups','{item.key if item.key_is_full else os.path.join(disk_conf.prefix, item.key)}','shadow',{item.size},1)" |
Contributor
There was a problem hiding this comment.
This logic was here but let's make missing_backups, shadow as function parameter here because is is named generally _insert_local_blobs_batch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Split object storage space usage accounting by replica and adjust schema and queries accordingly.
New Features:
Enhancements:
Tests: