[manager/service/client] add cache meta detail API - #225
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds a GetCacheMetaDetail diagnostic API that exposes raw KVCM metadata (all locations, status, block properties) without applying selection, filtering, or lazy pruning. The layered implementation is clean and consistent — proto definition → service → manager → meta_searcher → client stub → Python connector, with metrics and HTTP/gRPC registration all handled correctly.
Key findings:
- Duplicate type representation (
common.h:97): The client-side struct usesint32_t storage_typewhile the server uses the typedDataStorageType type, making cross-layer inspection error-prone. - Error path in
key_not_foundcheck (cache_manager.cc:418): Keys with unexpected non-OK, non-NOENT error codes fall through to the location-building path silently.BatchGetRawMetaabsorbs these errors, so callers get an empty-locations item with no indication of the underlying failure. - Duplicate data in proto response (
manager_message_proto_util.h:403):prev_block_keyandproperties["BP#prev_key"]both carry the same value. Intentional but undocumented. - Dead variable (
meta_service_impl.cc:488):invalid_fieldsis declared but never read. - No integration test assertions (
meta_interface_cases.py:44): Only stub plumbing is added; no test case actually calls and asserts on the new endpoint.
Location-ID lexicographic sorting (cache_manager.cc:436) is deterministic but undocumented as a contract.
🤖 Generated by Qoder
| struct CacheMetaLocationDetail { | ||
| std::string location_id; | ||
| CacheMetaLocationStatus status{CacheMetaLocationStatus::CLS_NOT_FOUND}; | ||
| int32_t storage_type{0}; |
There was a problem hiding this comment.
The client-side CacheMetaLocationDetail uses a raw int32_t storage_type field, while the server-side CacheLocationMetaDetail (in cache_location_view.h) uses the typed DataStorageType type. These two parallel structs diverge in type for the same semantic field. Consider using a typed enum (or at minimum matching the server struct's field name) to avoid silent misuse when callers inspect the storage type on the client side.
🤖 Generated by Qoder
| proto::meta::CacheMetaDetailItem *proto_cache_meta_detail) { | ||
| proto_cache_meta_detail->set_request_index(static_cast<int32_t>(cache_meta_detail.request_index)); | ||
| proto_cache_meta_detail->set_block_key(cache_meta_detail.block_key); | ||
| auto prev_key_iter = cache_meta_detail.properties.find(PROPERTY_PREV_BLOCK_KEY); |
There was a problem hiding this comment.
prev_block_key is populated by looking up PROPERTY_PREV_BLOCK_KEY in properties, and then the full properties map (which includes the same key) is also serialized into proto_cache_meta_detail->mutable_properties() below. This means prev_block_key and properties["BP#prev_key"] carry duplicate data in the response, as illustrated in the doc example. If prev_block_key is meant to be a convenience field, that's fine, but it's worth a comment confirming the duplication is intentional.
🤖 Generated by Qoder
| item.properties = properties[idx]; | ||
| } | ||
|
|
||
| const bool key_not_found = |
There was a problem hiding this comment.
The key_not_found condition checks per_key_ecs[idx] == ErrorCode::EC_NOENT but does not treat other non-OK error codes as "not found" — it falls through to the location-building path for them. However BatchGetRawMeta only warns on those unexpected errors and still returns EC_OK to the caller, so location_maps[idx] may be in an indeterminate state for those keys. It may be worth adding an explicit arm: if per_key_ecs[idx] != EC_OK && per_key_ecs[idx] != EC_NOENT, either record the error in the item or treat it as not-found, rather than silently producing an empty-locations item.
🤖 Generated by Qoder
| """Get cache location for specified block keys""" | ||
| return {} | ||
|
|
||
| @abc.abstractmethod |
There was a problem hiding this comment.
The abstract method has a default return {} body. Python's abc.abstractmethod does not enforce the override if a default body is provided; subclasses that forget to implement this will silently get an empty dict back rather than a clear TypeError. This is the same pattern as the existing methods, so it's consistent, but worth noting that test coverage depends on callers going through the concrete GrpcInterfaceTest/HttpInterfaceTest implementations. There are no test cases in this PR that actually exercise get_cache_meta_detail end-to-end through the integration test runner (only stub plumbing is added) — adding at least one basic call-and-assert test would close this gap.
🤖 Generated by Qoder
7d6f3d8 to
04c813c
Compare
wangxiyu191
changed the title
![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: 7d6f3d8b7c
ℹ️ 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".
| "get raw meta failed, key[%lu](%lu), error_code: %d", idx, keys[idx], out_error_codes[idx]); | ||
| } | ||
| } | ||
| return EC_OK; |
There was a problem hiding this comment.
When MetaIndexer::Get returns EC_TIMEOUT, EC_ERROR, or EC_PARTIAL_OK because of a backend/deserialize failure rather than a normal EC_NOENT miss, this method only logs the per-key error and still returns EC_OK; CacheManager::GetCacheMetaDetail then treats empty/missing maps as CLS_NOT_FOUND, so callers get a successful diagnostic response that falsely reports the key as absent. Please propagate a non-OK result, or expose per-key errors, for any non-EC_NOENT failure so storage outages are not reported as misses.
Useful? React with 👍 / 👎.
04c813c to
bd3dbbd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be9a2fe369
ℹ️ 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".
be9a2fe to
494581b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 494581b6f4
ℹ️ 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".
| for (size_t idx = 0; idx < all_keys.size(); ++idx) { | ||
| if (IsIndexInMaskRange(block_mask, idx)) { |
There was a problem hiding this comment.
When a caller supplies a nonempty boolean mask whose length differs from all_keys.size(), or an offset larger than the key count, this loop silently accepts it: missing boolean entries are treated as unmasked, while an oversized offset returns an empty successful response. The BlockMask contract requires boolean masks to match the key count, so validate it with IsBlockMaskValid (while preserving the implicit empty-mask case) before filtering; otherwise malformed diagnostic requests can query unintended keys or falsely report no results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0643028046
ℹ️ 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".
| proto::meta::GetCacheMetaDetailResponse response; | ||
| auto grpc_status = stub->GetCacheMetaDetail(&context, request, &response); | ||
| CHECK_GRPC_STATUS_WITH_TYPE(grpc_status); | ||
| CHECK_COMMON_HEADER_WITH_TYPE(response); |
There was a problem hiding this comment.
When every raw metadata lookup fails, the service deliberately returns a non-OK header together with populated per-key items, but this header check immediately returns {client_ec, {}} before GenCacheMetaDetails runs. Consequently C++ MatchMetaDetail callers lose the key indexes and per-item failure details precisely during a complete backend outage; preserve response.items() alongside the overall error.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests