[optimizer] consume KVCM event streams and report online MRC metrics - #296
[optimizer] consume KVCM event streams and report online MRC metrics#296Tyndalllll wants to merge 18 commits into
Conversation
991aa72 to
02c903f
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: 02c903f174
ℹ️ 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".
| } | ||
| auto query_scope = report_service_metrics ? metrics_collector.MakeServiceQueryScope() : ChronoScopeGuard{}; | ||
| proto::optimizer::TraceQueryResponse response; | ||
| const ErrorCode ec = optimizer_service_->ExecuteTraceQuery(event, &response); |
There was a problem hiding this comment.
When KVCM serves QT_BATCH_GET or QT_REVERSE_ROLL_SW_MATCH, this path replays the event unconditionally as a full-attention prefix query. CacheManager records query_type in CacheGetEvent, but OptimizerEventPublisher::Convert does not serialize it, and auto-created optimizer groups enable prefix hashing, so independent batch keys are rolling-hashed and interpreted as one prefix chain. For workloads using these query types, hit rates and MRC values are therefore unrelated to the actual accesses; either filter the stream to compatible prefix-match events or carry the query type through the protocol and handle each form correctly.
Useful? React with 👍 / 👎.
| if (source.location_spec_groups_size() > 1) { | ||
| KVCM_LOG_WARN("ApplyKvcmConfiguration: ignore unsupported multi-group instance[%s], groups=%d", | ||
| source.instance_id().c_str(), | ||
| source.location_spec_groups_size()); | ||
| unsupported_instance_ids.insert(source.instance_id()); | ||
| continue; |
There was a problem hiding this comment.
KVCM permits legacy instances to omit location_spec_groups—the field defaults to an empty vector—but this check only classifies instances with more than one group as unsupported. A zero-group instance therefore reaches OnlineOptimizerManager::RegisterInstance, which rejects it because implicit full-only registration requires exactly one group; ApplyKvcmConfiguration then fails the entire snapshot, SyncConfiguration never calls UpdateWorker, and no events are consumed even for otherwise valid instances. Treat zero-group instances as unsupported or synthesize a full group from their location_spec_infos.
Useful? React with 👍 / 👎.
| if (state->instance_group->enable_theoretical_max_cache()) { | ||
| const uint64_t max_hits = HitCurveProjector::ProjectInfinite(fact); | ||
| state->mrc_window.Record(fact); | ||
| result.max_hit_count = ClampToInt64(max_hits); |
There was a problem hiding this comment.
When metrics_report_interval_ms is zero or negative, OnlineOptimizerServer::Start intentionally creates no reporting thread, but every theoretical query still records into mrc_window. Because TakeMrcMetrics is then never called, its sparse map is never reset and retains every historical reuse-distance boundary, including entries for blocks later removed by the 24-hour TTL; a long-running optimizer with interval reporting disabled can therefore accumulate substantial unbounded historical metric state. Skip MRC recording in this mode or provide another bounded/reset path.
Useful? React with 👍 / 👎.
| CommonResponseHeader header = 1; | ||
| repeated int64 estimated_capacity_blocks = 2; // 各档容量按 average block size 折算的 block 数 | ||
| int64 size_full_only = 3; // 仅 full spec 的单 block 字节数 | ||
| int64 size_full = 3; // 仅 full spec 的单 block 字节数 |
There was a problem hiding this comment.
Renaming protobuf field 3 from size_full_only to size_full is wire-compatible for binary gRPC, but it breaks the public source and HTTP contracts: regenerated clients no longer have the size_full_only accessor, and this repository's JSON serializer uses preserve_proto_field_names, so /api/optimizer/registerInstance now returns size_full instead of size_full_only. Existing HTTP consumers that read the old field silently lose the block-size result; retain the protobuf name and limit the terminology rename to internal C++ state, or provide an explicit compatibility path.
Useful? React with 👍 / 👎.
| if (report_service_metrics) { | ||
| if (ec != EC_OK) { | ||
| metrics_collector.set_service_error_code_metrics(static_cast<double>(ec)); | ||
| } | ||
| metrics_reporter_->ReportPerQuery(&metrics_collector); |
There was a problem hiding this comment.
For an instance deliberately placed in unsupported_instance_ids_ because it has multiple location-spec groups, ExecuteTraceQuery returns EC_INSTANCE_NOT_EXIST, and this block reports it as a service error before the later unsupported-instance check suppresses the log and refresh. Every access to such a known unsupported instance therefore increments service.error_counter/error QPS, potentially dominating alerts in mixed-model deployments even though the subscriber is behaving as configured. Check the unsupported set before reporting the error, or classify these drops separately.
Useful? React with 👍 / 👎.
02c903f to
d99e00e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d99e00ecff
ℹ️ 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".
| KVCM_LOG_WARN("KvcmEventSubscriber: invalid leader endpoint returned by seed[%s]", endpoint.host.c_str()); | ||
| continue; | ||
| } | ||
| leader_endpoint = leader.host() + ":" + std::to_string(leader.meta_rpc_port()); |
There was a problem hiding this comment.
When GetClusterInfo returns an IPv6 literal, this produces an ambiguous target such as 2001:db8::1:6381 rather than the gRPC address [2001:db8::1]:6381. Consequently, an optimizer that successfully reaches an IPv6 seed cannot connect to the discovered leader for GetConfiguration or SubscribeEvents; format IPv6 hosts with brackets before constructing the endpoint.
Useful? React with 👍 / 👎.
Co-authored-by: putaopi <72243274+putaopi7@users.noreply.github.com>
d99e00e to
9d0ef1d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d0ef1dc9f
ℹ️ 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".
| const ErrorCode ec = | ||
| manager_->TraceQuery(request.instance_id(), block_keys, input_token_len, request.timestamp_ns(), result); |
There was a problem hiding this comment.
When a supported prefix query supplies per-key location_spec_names, this call discards them and replays only block_keys. KVCM treats the spec as part of a block's logical identity and uses each per-position name to select cache candidates, so accesses such as (key, F0) followed by (key, L1) can have different serving outcomes while the optimizer treats the latter as a hit on the former; the configured full-group byte charge is also inaccurate for such partial-spec accesses. This corrupts hit-rate and MRC results even without the unsupported query types noted elsewhere, so nonempty spec-qualified events should either be modeled with spec-aware identities/charges or excluded from replay.
Useful? React with 👍 / 👎.
| kmon_ctx_->mrc_metrics.reset(reporter->RegisterMetric("mrc", kmonitor::GAUGE, kmonitor::FATAL)); | ||
| if (!kmon_ctx_->mrc_metrics) { | ||
| KVCM_LOG_ERROR("failed to register metric:[mrc]"); | ||
| return false; |
There was a problem hiding this comment.
If KMonitor accepts the existing metrics but rejects the newly added mrc metric, this returns failure while leaving kmon_ctx_, its kmonitor pointer, and a null mrc_metrics intact. OnlineOptimizerServer::Init explicitly continues after InitKmonitor() returns false, and once a full-attention instance exists the next interval report passes its context checks and dereferences mrc_metrics in ReportInterval, crashing the optimizer instead of running with KMonitor disabled. Reset the KMonitor context on this failure path, as is already done for earlier initialization failures.
Useful? React with 👍 / 👎.
| if (source.name().empty() || source.capacity_bytes() <= 0) { | ||
| KVCM_LOG_ERROR("ApplyKvcmConfiguration: invalid KVCM instance group[%s], capacity_bytes=%ld", | ||
| source.name().c_str(), | ||
| source.capacity_bytes()); | ||
| return EC_BADARGS; |
There was a problem hiding this comment.
When a KVCM snapshot contains an instance group whose quota capacity is zero or negative, this rejects the entire snapshot. KVCM itself permits these capacities—InstanceGroupQuota::ValidateRequiredFields imposes no positivity constraint, and the serving/reclaimer paths interpret nonpositive capacity as a group with no available quota—while GetConfiguration emits every registered group. Thus one intentionally disabled group prevents SyncConfiguration from calling UpdateWorker, so events are not consumed for any otherwise valid positive-capacity group; classify the nonpositive group and its instances as unsupported rather than failing the complete synchronization.
Useful? React with 👍 / 👎.
Track interval hit rates, service input tokens, request latency totals, and errors for streamed queries. Add instance-group labels to per-instance metrics and move optional KMonitor reporting behind its own reporter.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d0fa11917
ℹ️ 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".
| if (!SyncConfiguration(leader_endpoint)) { | ||
| return; |
There was a problem hiding this comment.
If the first reachable seed returns a stale or temporarily unreachable leader endpoint, DiscoverLeader stops at that seed and this failure returns from the refresh without consulting the remaining healthy seeds. Because every refresh repeats the same ordering, a stale first seed can prevent configuration synchronization and event consumption indefinitely even when another seed reports a usable current leader; continue discovery with the remaining seeds when SyncConfiguration fails.
Useful? React with 👍 / 👎.
| { | ||
| OptimizerCallGuard guard(&request_context, metrics_reporter_.get()); | ||
| ec = optimizer_service_->ExecuteTraceQuery(event, &response); |
There was a problem hiding this comment.
When event subscription is enabled, every cache access passes through this OptimizerCallGuard, whose destructor unconditionally emits KVCM_ACCESS_LOG. Unlike an optimizer RPC, this path can run at the full KVCM cache-query rate, so it generates an additional access-log record per streamed event and can make logging I/O or disk volume dominate the single stream-processing thread, causing its bounded upstream queue to drop events. Keep the metrics/timing behavior without using the RPC guard's access-log side effect.
Useful? React with 👍 / 👎.
|
|
||
| const RequestFact fact = | ||
| state->lite_hit->ProcessRequest(normalized.block_keys, TimestampUtil::GetCurrentTimeUs() * 1000); | ||
| const RequestFact fact = state->lite_hit->ProcessRequest(normalized.block_keys, replay_timestamp_ns); |
There was a problem hiding this comment.
When timestamped events are delayed longer than an instance's TTL, replaying the first event here creates state at the producer timestamp, but the periodic ListInstances call advances the same LiteHit state to the optimizer's current wall clock. If reporting occurs before the next queued producer-time event, it expires that state, so an event at producer time t+5 misses data inserted at t even when the TTL is 10 seconds. This defeats the producer-timestamp semantics added for queueing delays; TTL maintenance and reporting need to use a consistent event-time watermark.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5516dc3c01
ℹ️ 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".
| std::uint64_t previous_sequence = 0; | ||
| bool has_previous_sequence = false; |
There was a problem hiding this comment.
When a producer resends a batch after losing the success response, this validation compares sequences only within the current request because previous_sequence is reset on every call and no progress is stored for producer_id. The repeated observations are therefore enqueued again, double-counting accesses and altering LRU/MRC results despite the sequence and last_accepted_sequence contract; retain the last accepted sequence per producer and reject or skip already accepted observations before enqueueing.
Useful? React with 👍 / 👎.
| for (const auto &observation : request->observations()) { | ||
| if (observation.trace_id().empty() || observation.instance_id().empty() || | ||
| observation.token_ids().empty() || | ||
| (has_previous_sequence && observation.sequence() <= previous_sequence)) { |
There was a problem hiding this comment.
When an observation supplies a negative timestamp_ns, this validation accepts it and the response counts it as successfully enqueued, but OnlineOptimizerManager::TraceQuery rejects every negative timestamp with EC_BADARGS. The subscriber therefore drops the observation asynchronously after the producer has been told it was accepted; validate the timestamp here so the whole invalid batch is rejected before SendBatch.
Useful? React with 👍 / 👎.
| if (!IsAvailable()) { | ||
| status->set_code(proto::optimizer::SERVICE_NOT_READY); | ||
| status->set_message("KVCM is unavailable"); |
There was a problem hiding this comment.
When report_trace_batch reaches a stable follower after a leader change, this branch returns SERVICE_NOT_READY, while KvCacheManagerClient._make_api_request only rediscovers the leader and retries on SERVER_NOT_LEADER. The public client therefore fails this observation call even when a healthy leader is available; return SERVER_NOT_LEADER for the follower case and reserve SERVICE_NOT_READY for recovery or queue availability failures.
Useful? React with 👍 / 👎.
2 participants
Summary
Attribution
Co-authored-byattributionPR stack
feat/optimizer-event-streamValidation