[Bug] fixed bug of master not using glog actually #1075
Conversation
Summary of ChangesHello @SpecterCipher, 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 resolves a bug in the mooncake master process where logging occurred before the proper initialization of Google Logging (glog). The changes ensure that glog is correctly set up and shut down, eliminating a startup warning and enabling all logs to be written in the intended glog format, with options for custom configuration via glog flags. 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
|
![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 aims to integrate glog for logging in the master service. The changes correctly include the necessary headers and add initialization and shutdown calls for glog. However, there are two issues with the implementation. First, glog::InitGoogleLogging() is called after gflags::ParseCommandLineFlags(), which does not fix the original issue of logging before initialization, as gflags validators may use glog. Second, the google::ShutdownGoogleLogging() call is unreachable, meaning glog resources won't be cleaned up properly on exit. My review includes comments with high and critical severity to address these points and suggests corrections.
mooncake-store/src/master.cpp
Outdated
| mooncake::RegisterRpcService(server, wrapped_master_service); | ||
| return server.start(); | ||
| } | ||
| google::ShutdownGoogleLogging(); |
There was a problem hiding this comment.
This call to google::ShutdownGoogleLogging() is unreachable. The if-else block starting at line 461 has return statements in both branches (return supervisor.Start(); at line 464 and return server.start(); at line 481), so the program will exit before this line is ever reached. This means glog resources will not be properly shut down, which can lead to logs not being flushed correctly.
To fix this, you should ensure google::ShutdownGoogleLogging() is called before all normal exit points of the main function. A common and robust pattern for this is to use an RAII object for initialization and cleanup at the top of main.
mooncake-store/src/master.cpp
Outdated
| gflags::ParseCommandLineFlags(&argc, &argv, true); | ||
|
|
||
| glog::InitGoogleLogging(argv[0]); |
There was a problem hiding this comment.
To fix the warning "Logging before InitGoogleLogging() is written to STDERR", glog::InitGoogleLogging should be called before gflags::ParseCommandLineFlags. Gflags validators, like the one defined on line 56 which uses LOG(FATAL), are executed during parsing. If a validator uses LOG(), glog must be initialized to avoid the warning and ensure logs are written to the configured files instead of STDERR. While glog uses gflags for its own configuration, InitGoogleLogging can be called before parsing, and glog will pick up the flag values after they are parsed.
| gflags::ParseCommandLineFlags(&argc, &argv, true); | |
| glog::InitGoogleLogging(argv[0]); | |
| glog::InitGoogleLogging(argv[0]); | |
| // Initialize gflags | |
| gflags::ParseCommandLineFlags(&argc, &argv, true); |
SpecterCipher
force-pushed
the
dev/SpecterCipher
branch
from
45dd4c1 to
c737a90
Compare
| // Initialize gflags | ||
| gflags::ParseCommandLineFlags(&argc, &argv, true); | ||
|
|
||
| GlogRAII glogRAII(argv[0]); |
There was a problem hiding this comment.
Consider using a simple if...else statement to enable glog.
|
Use clang-format -i FILENAME before submiting PR. |
|
Why use RAII for glog? @SpecterCipher |
2 participants
Description
Before modification, when the mooncake master started, it would issue a warning: "Logging before InitGoogleLogging() is written to STDERR". After modification, logs are printed in the glog format, defaulting to the /tmp directory with filenames starting with mooncake_master. Custom log configuration can be achieved by setting the corresponding glog flags during startup.
Type of Change