Conversation
2f091d0 to
232203b
Compare
There was a problem hiding this comment.
👋 Review Summary
Nice work threading GetCacheLocationLen all the way from proto through service, manager, stub, and client, and adding tests at each layer. The plumbing, validation, and logging patterns are consistent with the existing GetCacheLocation/GetCacheMeta endpoints.
🛡️ Key Risks & Issues
- Hit-count semantics across QueryType values:
CacheManager::GetCacheLocationLencurrently returnscache_locations.size()ascache_location_lenfor allQueryTypevalues and forwards that to the client/API as “命中的key的数量”. ForQT_PREFIX_MATCHthis matches the intended semantics, but forQT_BATCH_GETandQT_REVERSE_ROLL_SW_MATCHthe underlying searchers sizeout_locationsto the number of requested keys and leave empty entries for misses. That meanscache_location_len(and themanager.prefix_match_lenmetric it feeds) will effectively report the number of queried keys rather than the number of real hits for non-prefix queries, which can mislead any caller or observability consumer that assumes it is a hit-count. It would be good to either explicitly constrain this API to prefix match or adjust the counting logic and metrics to reflect actual hits for all supportedQueryTypevalues.
🧪 Verification Advice
- Beyond the existing tests for
QT_PREFIX_MATCH, consider adding targeted tests that exerciseGetCacheLocationLenwithQT_BATCH_GETandQT_REVERSE_ROLL_SW_MATCH, including mixed hit/miss scenarios and tokens-only queries, and cross-check the returnedcache_location_lenagainstGetCacheLocationfor the same inputs.
💡 Thoughts & Suggestions
- The reuse of existing validation (
CheckInputAndGetMetaSearcher,GetBlockSize,GenKeyVector), metrics hooks, and event publishing is a strong choice and should make this endpoint easier to operate in production. Once the semantics for non-prefix query types are clarified and enforced (either by restriction or adjusted counting), this new API will be a nice complement toGetCacheLocationfor lightweight hit-length checks.
🤖 Generated by Qoder • View workflow run
|
|
||
| RETURN_IF_EC_NOT_OK_WITH_TYPE_LOG(WARN, ec, int64_t, "get cache location length failed"); | ||
| int64_t cache_location_len = static_cast<int64_t>(cache_locations.size()); |
There was a problem hiding this comment.
In CacheManager::GetCacheLocationLen the value cache_location_len is computed as cache_locations.size() for all QueryType values. This matches the "命中的key的数量" semantics only for prefix-style queries where every element in cache_locations represents a real hit.
For other query types the same assumption does not hold. For example, in the BatchGet path the implementation reserves out_locations to keys.size() and pushes one entry per key, inserting an empty CacheLocation for misses. In reverse-roll slide-window match, the vector is pre-sized to keys.size() and only a subset of positions is filled for a successful window. In both cases, cache_locations.size() equals the number of queried keys, not the number of keys that actually have valid locations, so cache_location_len (and the associated prefix_match_len metric) will over-report hits for non-prefix queries.
If the intent is "number of hit keys" as described in the proto comment, we probably need to either (a) restrict this API to QT_PREFIX_MATCH and enforce that at the service layer, or (b) compute the length by counting only entries that represent real hits (e.g. non-empty locations) and adjust the metric naming so it is not prefix-specific when other query types are supported.
🤖 Generated by Qoder • Fix in Qoder
10696d2 to
c83a114
Compare
No description provided.