[Store] Fix error log spam for non-memory replicas in DiscardedReplicas#1626
Conversation
DiscardedReplicas iterates all replicas and calls get_memory_buffer_size(), which logs "Invalid replica type: DISK" for non-memory replicas. In L3/DFS scenarios where DISK replicas are routinely discarded, this produces continuous error log spam. Guard the call with is_memory_replica() so only memory replicas contribute to the size counter. Functionally equivalent (non-memory types already return 0) but eliminates the misleading error logs. Also add LOCAL_DISK to the ReplicaType operator<< string map — it was missing, causing LOCAL_DISK to print as "UNKNOWN" in diagnostics. Fixes kvcache-ai#1618
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue causing continuous error log spam in L3/DFS scenarios by preventing an invalid method call on non-memory replicas. It also improves logging clarity by ensuring all replica types are correctly represented in string output. The changes aim to reduce noise in system logs and enhance debugging capabilities without altering core functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the error log spam from DiscardedReplicas by adding a guard to prevent calls to get_memory_buffer_size() for non-memory replicas. It also correctly adds the missing LOCAL_DISK replica type to its string conversion operator, fixing an incorrect log output. The changes are sound and well-targeted. I've added one suggestion to improve the performance of the enum-to-string conversion in replica.h by using a more efficient data structure.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Clear fix. LGTM. THX |
…as (kvcache-ai#1626) DiscardedReplicas iterates all replicas and calls get_memory_buffer_size(), which logs "Invalid replica type: DISK" for non-memory replicas. In L3/DFS scenarios where DISK replicas are routinely discarded, this produces continuous error log spam. Guard the call with is_memory_replica() so only memory replicas contribute to the size counter. Functionally equivalent (non-memory types already return 0) but eliminates the misleading error logs. Also add LOCAL_DISK to the ReplicaType operator<< string map — it was missing, causing LOCAL_DISK to print as "UNKNOWN" in diagnostics. Fixes kvcache-ai#1618
Fixes #1618
Root Cause
DiscardedReplicas(master_service.h) iterates all discarded replicas and callsget_memory_buffer_size()on each. This method only handlesMEMORYreplicas — forDISKorLOCAL_DISK, it logs"Invalid replica type: DISK"and returns 0.In L3/DFS scenarios where
DISKreplicas are routinely discarded, this produces continuous error log spam (as reported in #1618).Changes
master_service.h: Guard
get_memory_buffer_size()call withis_memory_replica()inDiscardedReplicasconstructor. Functionally equivalent (non-memory types already return 0) but eliminates the error log.replica.h: Add
LOCAL_DISKto theoperator<<string map forReplicaType. It was missing — printing aLOCAL_DISKtype showed "UNKNOWN" instead of "LOCAL_DISK".