[Store] feat: Implement a FileStorage component to manage the lifecycle of key-value data #1031
Conversation
…le of key-value data, including creation, reading, and synchronizing metadata with the master.
Summary of ChangesHello @zhuxinjie-nz, 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 introduces a new 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 introduces a new FileStorage component to manage the lifecycle of key-value data, including storage, retrieval, and offloading. The implementation is well-structured, with a dedicated configuration class, a FileStorage class to orchestrate operations, and a BucketIterator. The related changes to storage_backend are consistent and improve correctness with new checks. The addition of unit tests for the new configuration is also a positive step.
However, the review identified several critical issues concerning the handling of tl::expected return values. In multiple places, the code calls .value() without first checking if an error is present, which will lead to application crashes. There is also a high-severity portability issue in the test code related to environment variable handling on Windows, and some medium-severity typos. Addressing these issues is crucial for the stability and correctness of the new component.
| if (!allocate_res) { | ||
| LOG(ERROR) << "Failed to allocate batch objects, target = " | ||
| << transfer_engine_addr; | ||
| } |
There was a problem hiding this comment.
The code checks if allocate_res contains an error and logs it, but then proceeds to call allocate_res.value() on the next line. This will cause a crash if allocate_res is an error. The function should return early in the error case.
| if (!allocate_res) { | |
| LOG(ERROR) << "Failed to allocate batch objects, target = " | |
| << transfer_engine_addr; | |
| } | |
| if (!allocate_res) { | |
| LOG(ERROR) << "Failed to allocate batch objects, target = " | |
| << transfer_engine_addr; | |
| return tl::make_unexpected(allocate_res.error()); | |
| } |
| if (!enable_offloading_result) { | ||
| LOG(ERROR) << "Get is enable offloading failed with error: " | ||
| << enable_offloading_result.error(); | ||
| } |
There was a problem hiding this comment.
If enable_offloading_result contains an error, it is logged, but the code proceeds to call .value() on it, which will cause a crash. The error should be handled, for example by returning from the function.
if (!enable_offloading_result) {
LOG(ERROR) << "Get is enable offloading failed with error: "
<< enable_offloading_result.error();
return tl::make_unexpected(enable_offloading_result.error());
}| if (!is_exist_result) { | ||
| LOG(ERROR) << "Failed to check existence in storage backend: " | ||
| << "key=" << it->first | ||
| << ", error=" << is_exist_result.error(); | ||
| } |
There was a problem hiding this comment.
If is_exist_result contains an error, it is logged, but the code proceeds to call .value() on it, which will cause a crash. The error should be handled. You could propagate the error or continue to the next item in the loop.
if (!is_exist_result) {
LOG(ERROR) << "Failed to check existence in storage backend: "
<< "key=" << it->first
<< ", error=" << is_exist_result.error();
return tl::make_unexpected(is_exist_result.error());
}
mooncake-store/src/file_storage.cpp
Outdated
|
|
||
| BucketIterator bucket_iterator(storage_backend_, | ||
| config_.bucket_iterator_keys_limit); | ||
| while (bucket_iterator.HasNext().value()) { |
There was a problem hiding this comment.
Calling .value() on a tl::expected object without first checking for an error can cause a crash if an error is present. The bucket_iterator.HasNext() method returns a tl::expected<bool, ErrorCode>, which should be checked before accessing its value.
A safer way to write this loop would be:
while (true) {
auto has_next_res = bucket_iterator.HasNext();
if (!has_next_res) {
LOG(ERROR) << "Failed to check for next bucket: " << has_next_res.error();
return tl::make_unexpected(has_next_res.error());
}
if (!has_next_res.value()) {
break;
}
// ... rest of the loop body
}| void UnsetEnv(const std::string& key) { | ||
| #ifdef _WIN32 | ||
| unsetenv(key.c_str()); | ||
| #else | ||
| unsetenv(key.c_str()); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
The unsetenv function is not standard on Windows. The current implementation uses unsetenv for both Windows (_WIN32) and other platforms, which will cause a compilation failure on Windows. To unset an environment variable on Windows, you should use _putenv_s(key.c_str(), "");.
| void UnsetEnv(const std::string& key) { | |
| #ifdef _WIN32 | |
| unsetenv(key.c_str()); | |
| #else | |
| unsetenv(key.c_str()); | |
| #endif | |
| } | |
| void UnsetEnv(const std::string& key) { | |
| #ifdef _WIN32 | |
| _putenv_s(key.c_str(), ""); | |
| #else | |
| unsetenv(key.c_str()); | |
| #endif | |
| } |
| /** | ||
| * @brief Groups offloading keys into buckets based on size and existence | ||
| * checks. | ||
| * @param @param offloading_objects Input map of object keys and their sizes |
There was a problem hiding this comment.
There is a duplicate @param tag in the Doxygen comment.
* @param offloading_objects Input map of object keys and their sizes| const std::vector<std::string>& keys); | ||
|
|
||
| tl::expected<void, ErrorCode> BatchLoad( | ||
| const std::unordered_map<std::string, Slice>& batche_object); |
There was a problem hiding this comment.
There is a typo in the parameter name batche_object. It should be batch_object.
const std::unordered_map<std::string, Slice>& batch_object);
mooncake-store/src/file_storage.cpp
Outdated
| } | ||
|
|
||
| tl::expected<void, ErrorCode> FileStorage::BatchLoad( | ||
| const std::unordered_map<std::string, Slice>& batche_object) { |
There was a problem hiding this comment.
There is a typo in the parameter name batche_object. It should be batch_object to be consistent with the rest of the codebase and to match the fix in the header file.
const std::unordered_map<std::string, Slice>& batch_object) {
PR Review SummaryI've completed a thorough review of this FileStorage implementation. Overall, the architecture is solid, but there are critical issues that must be addressed before merging. 🔴 Critical Issues (P0 - Must Fix)1. Stub Implementations - Blocking IssueFour critical Client methods are not implemented (mooncake-store/src/client.cpp:1524-1554):
All these methods just return Impact: FileStorage::Init() calls MountFileStorage at line 141, FileStorage::Heartbeat() calls OffloadObjectHeartbeat at line 296, etc. These are core functionalities. 2. Thread Safety ViolationIn auto heartbeat_result = client_->OffloadObjectHeartbeat(
segment_name_, enable_offloading_, offloading_objects); // Race condition!The field is marked Fix: Acquire the mutex before reading, or copy the value under lock. 3. Silent Error SwallowingIn auto result = BatchOffload(keys);
if (!result) {
LOG(ERROR) << "Failed to store objects with error: " << result.error();
}
// Function continues and returns {} despite error!Fix: Return the error to the caller. 🟡 Important Issues (P1 - Should Fix)4. Typo in Error Code
5. Missing Integration Tests
6. Security: Path Validation
7. Type InconsistencyIn for (int64_t i = 0; i < keys.size(); ++i) { // Type mismatch✅ Strengths
📝 Recommendations (P2 - Nice to Have)
VerdictStatus: The architecture and design are solid, but the stub implementations make this non-functional. Additionally, the thread safety issue could cause production incidents. Estimated work: 1-2 days to implement the four Client methods + fix critical issues, plus another day for comprehensive testing. Once P0 items are resolved, this will be a strong addition to the codebase! 👍 |
|
Client APIs are ready but will be submitted in the next PR to keep changes focused. FileStorage currently has no external read/write surface — this change is safe and non-breaking. |
…or metadata changes
2 participants
The FileStorage class will be responsible for managing the entire lifecycle of key-value objects, including: