-
Notifications
You must be signed in to change notification settings - Fork 460
[Store] refactor: config loading and CLI parsing #1174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @cocktail828, 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 undertakes a significant refactoring of the Mooncake Store's configuration loading and command-line interface parsing. The primary motivation was to address the limitations of the existing Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a significant and valuable refactoring of the configuration and command-line parsing logic. It greatly simplifies the configuration structure by replacing multiple config classes with a single MasterConfig and leveraging CLI11 for parsing, which is a commendable improvement. The introduction of a dedicated GlogConfig for logging is also a positive step towards better production-readiness.
My review focuses on a few areas where the refactoring might have introduced issues or could be further improved. I've identified a critical issue related to High Availability where the view_version is no longer propagated, which could impact client behavior. There are also a couple of high-severity issues with command-line option parsing and error handling. Additionally, I've included a suggestion for a minor performance improvement in the logging initialization.
Overall, this is an excellent pull request that enhances code quality and maintainability. Addressing the identified issues will help ensure the refactoring is complete and robust.
| LOG(INFO) << "Starting master service..."; | ||
| mooncake::WrappedMasterService wrapped_master_service( | ||
| mooncake::WrappedMasterServiceConfig(config_, view_version)); | ||
| mooncake::WrappedMasterService wrapped_master_service(config_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The view_version obtained from mv_helper.ElectLeader is not being passed to the WrappedMasterService. In the previous implementation, this was handled by WrappedMasterServiceConfig. After the refactoring, WrappedMasterService's constructor only takes MasterConfig, and MasterConfig no longer has a view_version field. This means MasterService::view_version_ will be default-initialized to 0, which is incorrect for HA mode. The view version is critical for clients to detect master changes. This appears to be a critical regression.
A possible fix is to add view_version back to MasterConfig and initialize it in the MasterService constructor, or to change the constructors of WrappedMasterService and MasterService to accept the view_version as a separate argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I’m aware, this variable doesn’t seem to be used anywhere in the code.
Description
The current command-line parsing uses gflags, which doesn't support hot updates and has limited extensibility. Additionally, the various config structure definitions and initialization code are verbose and difficult to read. Furthermore, logging has never supported persistence and always prints to screen, making it unsuitable for production environments.
This PR replaces gflags with CLI11, which allows configuration and command-line parsing to be handled together while flexibly supporting parameter deprecation mechanisms. It also makes configuration hot-reloading possible with minimal overhead - no need to write redundant code for parsing configuration files, just re-call the Parser interface. Priority mechanisms are also supported (Command Line > Configuration File > Default Values).
Additionally, duplicate code related to Master config has been refactored and streamlined. This modification significantly simplifies master-related code.
Type of Change
How Has This Been Tested?
Checklist