Skip to content

[manager] skip empty spec storage allocation - #309

Open
lucky-zzz wants to merge 1 commit into
mainfrom
fix/skip-empty-spec-allocation
Open

[manager] skip empty spec storage allocation#309
lucky-zzz wants to merge 1 commit into
mainfrom
fix/skip-empty-spec-allocation

Conversation

@lucky-zzz

Copy link
Copy Markdown
Collaborator

Summary

  • skip storage allocation for registered location specs that have no matching block keys
  • avoid invalid PACE batch requests with count=0 when StartWriteCache selects only a subset of location spec groups

Testing

  • not rerun after preparing the final commit; PR CI will validate the change

@wangxiyu191 wangxiyu191 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Correctness — The change is correct and minimal. When StartWriteCache passes location_spec_group_names that select only a subset of spec groups, specs outside the requested groups previously built an empty block_keys and still invoked data_storage_manager->Create, sending a zero-count batch to the backend. For in-tree backends (dummy/mooncake/nfs) the empty call was already benign — empty result, and the results.size() != block_keys.size() check evaluates 0 == 0 — so the skip is behavior-preserving; it also avoids spurious create_keys_qps samples with 0 keys and a needless rw_lock_ acquisition.

Findings

  • Inline: the same empty-batch call is still reachable via CreateInSingleBatch (cache_manager.cc:2049) when no registered spec matches the requested groups (that path is taken whenever all spec sizes are equal). A guard there — or an early return {} for empty keys in DataStorageManager::Create, matching Delete's existing empty-input guard — would close the gap for all call sites.

Testing

  • No targeted test covers a spec excluded by group filtering producing an empty block_keys (existing spec-group tests like TestFilterWriteCacheTieredMarkChecksSpecGroupOnTarget don't exercise the CreateBySpec skip path). Since observable behavior is unchanged for in-tree backends, this is low risk, but a small regression test would lock in the skip semantics. Relying on PR CI per the description is acceptable given the size of the change.

🤖 Generated by Qoder

Comment on lines +2117 to +2119
if (block_keys.empty()) {
continue;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This guard fixes the per-spec path, but the same empty-batch call is still reachable in the merged path: CreateInSingleBatch (cache_manager.cc:2049) calls data_storage_manager->Create with merged_block_keys, which is also empty when none of the registered specs match the requested location_spec_group_names — and that path is chosen whenever all spec sizes are equal (the common case). location_spec_infos() being empty also lands there with an empty key vector and common_size == 0.

Two options, either is behavior-preserving for in-tree backends (empty result, results.size() != block_keys.size()0 == 0):

  • add the same empty guard before the Create call at cache_manager.cc:2049, or
  • handle it once in DataStorageManager::Create with an early return {} for empty keys, mirroring the existing empty-input guard in Delete (data_storage_manager.cc:235) — this also covers every other caller (e.g. migration_manager) for free.

Just flagging the gap since the count=0 scenario this PR targets can still occur via the merged path.


🤖 Generated by Qoder

@github-actions github-actions Bot added the ai reviewed AI has reviewed this PR label Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai reviewed AI has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants