[proto] declare ReportEvent snapshot schema#235
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The snapshot-schema placeholder is reasonable and the comments correctly warn that EVENT_BLOCK_SNAPSHOT is not yet usable. Two concerns:
-
Breaking renames — Renaming every existing
*Paramsmessage to*EventParamsis a breaking change for all generated-code consumers. This feels broader than a "schema-only, snapshot-focused" PR. If this is just aligning with PR #230, please make that explicit in the summary. -
Unimplemented semantics —
BlockDeleteEventParams.spec_namesdescribes runtime filtering behavior, but the PR is schema-only. Clarify whether the server already honors this field or if it should be treated as currently ignored, similar to theEVENT_BLOCK_SNAPSHOTwarning.
🤖 Generated by Qoder
| NodeRegisterEventParams node_register = 2; | ||
| BlockAddEventParams block_add = 3; | ||
| BlockDeleteEventParams block_delete = 4; | ||
| HostDownEventParams host_down = 5; | ||
| HeartbeatEventParams heartbeat = 6; | ||
| // Declared for schema compatibility only; EVENT_BLOCK_SNAPSHOT is not | ||
| // usable until its server implementation lands. | ||
| BlockSnapshotEventParams block_snapshot = 7; |
There was a problem hiding this comment.
Renaming all existing *Params messages to *EventParams is a breaking change for any generated-code consumers (C++/Python/Go clients and the server). Since the PR description says this is a schema-only, snapshot-focused change, the scope of these renames feels broader than necessary. If PR #230 already introduced these names and this is just aligning, please mention that in the summary so reviewers know the breakage is intentional and pre-negotiated. Otherwise, consider keeping the original names and only adding the new snapshot types.
🤖 Generated by Qoder
| // Optional. When set, KVCM applies the delete only if these component names | ||
| // contain a full-attention spec; mamba-state-only deletes are ignored. | ||
| repeated string spec_names = 3; |
There was a problem hiding this comment.
The comment describes runtime behavior ("KVCM applies the delete only if these component names contain a full-attention spec"), but this PR is schema-only. If the server does not yet honor spec_names, the comment could mislead callers into expecting filtering that does not exist. Please either clarify that this field is currently ignored (like EVENT_BLOCK_SNAPSHOT) or confirm that the matching implementation is already present elsewhere.
🤖 Generated by Qoder
5ef5d2b to
c5a9b98
Compare
c5a9b98 to
76039c6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a9b98efb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Optional. When set, KVCM applies the delete only if these component names | ||
| // contain a full-attention spec; mamba-state-only deletes are ignored. | ||
| repeated string spec_names = 3; |
There was a problem hiding this comment.
When a reporter sends EVENT_BLOCK_DELETE with spec_names set to only mamba-state names, this new schema promises that KVCM will ignore the delete, but the current ReportEvent path never reads p.spec_names() and always queues BuildLocationId(p.medium(), host_ip_port) for deletion in cache_manager.cc:1619-1620. That removes the entire host/medium location, including any full-attention specs previously added under the same location, so clients following the newly advertised field can accidentally evict valid KV cache metadata.
Useful? React with 👍 / 👎.
| message NodeRegisterEventParams { | ||
| repeated string mediums = 1; // mediums supported by the node, e.g. ["mem","disk"] | ||
| } | ||
|
|
||
| message BlockAddParams { | ||
| message BlockAddEventParams { |
There was a problem hiding this comment.
Renaming the existing protobuf messages changes the generated API surface even though the field numbers and oneof accessors stay wire-compatible. Downstream ReportEvent producers that construct or annotate NodeRegisterParams, BlockAddParams, etc. from the current schema will fail to compile/import after regenerating stubs because those public types have been replaced by *EventParams; keeping the old message names while adding the snapshot types avoids a source-breaking schema-only change.
Useful? React with 👍 / 👎.
| // where each cache component of the block lives. KVCM currently matches only | ||
| // full-attention specs and ignores mamba-state specs by LocationSpec.name. |
There was a problem hiding this comment.
This comment creates an API contract that mamba-state LocationSpec.names are ignored, but the current EVENT_BLOCK_ADD implementation copies every reported spec into the upsert list (cache_manager.cc:1589-1595) with no name filtering. If a cache subscriber follows this schema text and reports mamba-only or mixed specs, KVCM can index and later return those non-full-attention components as cache metadata instead of ignoring them.
Useful? React with 👍 / 👎.
wangxiyu191
changed the title
2 participants
Summary
kv_cache_manager/protocol/protobuf/meta_service.protowith the schema shape from Add ReportEvent block snapshot reconciliation #230.NodeRegisterEventParams,BlockAddEventParams,BlockDeleteEventParams,HostDownEventParams, andHeartbeatEventParams.EVENT_BLOCK_SNAPSHOT,BlockSnapshotItem,BlockSnapshotEventParams, and theblock_snapshot = 7oneof branch.BlockDeleteEventParams.spec_names = 3as declared by Add ReportEvent block snapshot reconciliation #230.EVENT_BLOCK_SNAPSHOTand verifies the current server behavior is a safe rejection (INVALID_ARGUMENTitem result), not an accidental implementation.Design / Implementation status
EVENT_BLOCK_SNAPSHOTis still unavailable on the server side in this PR; callers must not send it until the implementation PR lands.BlockDeleteEventParams.spec_namesis included to match Add ReportEvent block snapshot reconciliation #230's schema; this PR does not change the current server-side delete behavior.item.block_add()/item.block_delete(); the oneof field names and numbers are unchanged.Validation
git diff --check11.122.133.161,screen -x qisa, window 3, commitfb2290a:git diff --check HEAD^git diff --exit-code origin/pr/230 -- kv_cache_manager/protocol/protobuf/meta_service.protobazelisk --output_base=/home/qisa.cb/workdir/bazel/kvcm-code3-proto230-verify build --config=debug //kv_cache_manager/manager:cache_manager //kv_cache_manager/service:meta_service_impl //kv_cache_manager/service:servicebazelisk --output_base=/home/qisa.cb/workdir/bazel/kvcm-code3-proto230-verify test --config=debug --config=asan --test_env ASAN_OPTIONS=detect_odr_violation=0 //kv_cache_manager/manager/test:CacheManagerTestCacheManagerTestPASSED,TEST_EXIT=0