add script for metadata management - #1746
Conversation
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 delivers a dedicated command-line interface to streamline Mooncake metadata management. It consolidates various key operations into a single, reliable tool, enhancing the safety and consistency of metadata interactions by prioritizing RPC-based methods and providing a more user-friendly experience for administrators. 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. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Python CLI tool, mc_meta_cli.py, designed to interact with Mooncake metadata. The tool supports listing all keys from a master HTTP debug API, querying specific keys via an HTTP metadata server, and deleting single or all keys using RPC. The review feedback suggests several improvements, including changing the HTTP response decoding error handling from ignore to replace for better data visibility, returning actual boolean values instead of string literals from query_key_by_http, enhancing type hints for the setup_store function, refactoring magic numbers for buffer sizes into module-level constants for improved maintainability, and making the default master server address more portable by using localhost.
| req = urllib.request.Request(url, method="GET") | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=timeout) as resp: | ||
| text = resp.read().decode("utf-8", errors="ignore") |
There was a problem hiding this comment.
Using errors="ignore" during decoding can silently drop malformed characters, potentially hiding issues with the data source. For a CLI tool that displays data, it's often better to use errors="replace" to make it clear that some data was corrupted, without crashing the script.
| text = resp.read().decode("utf-8", errors="ignore") | |
| text = resp.read().decode("utf-8", errors="replace") |
| ) from exc | ||
|
|
||
|
|
||
| def setup_store(args: argparse.Namespace) -> Any: |
There was a problem hiding this comment.
The return type of this function is Any, which is not very specific. For better type safety and readability, you can use a TYPE_CHECKING block to import MooncakeDistributedStore for type hinting. This avoids runtime import issues while providing static type checkers with the necessary information.
- Add the following at the top of the file with other imports:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from mooncake.store import MooncakeDistributedStore- Then, change this function's signature to use the specific type:
| def setup_store(args: argparse.Namespace) -> Any: | |
| def setup_store(args: argparse.Namespace) -> "MooncakeDistributedStore": |
| if args.local_buffer_size == 128 * 1024 * 1024: | ||
| args.local_buffer_size = 0 | ||
| if args.global_segment_size == 512 * 1024 * 1024: |
There was a problem hiding this comment.
These magic numbers for buffer sizes are also used as defaults in build_parser. To avoid duplication and improve maintainability, you should define them as constants at the module level and use them in both places. This will prevent potential bugs if the default values are changed in one place but not the other.
For example, you could add this at the top of your file:
# Constants for memory sizes
MIB = 1024 * 1024
DEFAULT_LOCAL_BUFFER_SIZE = 128 * MIB
DEFAULT_GLOBAL_SEGMENT_SIZE = 512 * MIB| if args.local_buffer_size == 128 * 1024 * 1024: | |
| args.local_buffer_size = 0 | |
| if args.global_segment_size == 512 * 1024 * 1024: | |
| if args.local_buffer_size == DEFAULT_LOCAL_BUFFER_SIZE: | |
| args.local_buffer_size = 0 | |
| if args.global_segment_size == DEFAULT_GLOBAL_SEGMENT_SIZE: |
| "--global-segment-size", | ||
| type=int, | ||
| default=512 * 1024 * 1024, | ||
| help="store.setup global_segment_size for RPC delete.", | ||
| ) | ||
| parser.add_argument( | ||
| "--local-buffer-size", | ||
| type=int, | ||
| default=128 * 1024 * 1024, | ||
| help="store.setup local_buffer_size for RPC delete.", |
There was a problem hiding this comment.
To complement the change in tune_rpc_only_memory, use the newly defined constants for the default values here. This improves readability and ensures consistency.
parser.add_argument(
"--global-segment-size",
type=int,
default=DEFAULT_GLOBAL_SEGMENT_SIZE,
help="store.setup global_segment_size for RPC delete.",
)
parser.add_argument(
"--local-buffer-size",
type=int,
default=DEFAULT_LOCAL_BUFFER_SIZE,
help="store.setup local_buffer_size for RPC delete.",
)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
3 participants
Summary
mc_meta_cli.pyas a Mooncake metadata client management utility to centralize key-management operations.rpc_only) by default to avoid hard dependency on the HTTP metadata service.Why