fix: build info not add to code, put the git information in cmake #49
Conversation
WalkthroughThe changes introduce new variables in the build configuration to capture the current Git commit hash and the build date. The build script is updated to remove outdated variables related to commit information. Additionally, the include directives in several source files are reorganized for better structure, with a change in how the time library is included. Overall, these modifications streamline the build process and improve code organization. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsFiles that changed from the base of the PR and between 228da1c917fc3c223bb47232091a753fe88607f6 and ba181bc. 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lqxhub
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (21)
src/pstd/memory_file.h (2)
Line range hint
134-138: Good addition ofSync()method.The introduction of a
Sync()method is a valuable addition for ensuring data integrity when working with memory-mapped files. The method's boolean return type appropriately allows for error handling.Consider expanding the comment slightly to clarify what "synchronize" means in this context. For example:
/*------------------------ * Sync() * Flush memory content to disk, ensuring all modifications * are written to the underlying file. Returns true if successful. */
Line range hint
1-224: Overall improvement in documentation and functionality.The changes in this file consistently enhance the clarity and completeness of the
OutputMemoryFileclass documentation. The addition of theSync()method provides important functionality for ensuring data integrity when working with memory-mapped files. These improvements will make the class easier to use and understand for developers.Consider adding error handling mechanisms or documentation about potential exceptions that might be thrown by these methods, especially for operations like
Sync(),Truncate(), andWrite()that interact with the file system. This would further improve the robustness and usability of the class.CMakeLists.txt (4)
34-38: LGTM! Consider adding error handling.The implementation to capture the Git commit hash is correct and follows good practices. However, it assumes that Git is installed and the directory is a Git repository.
Consider adding error handling to gracefully manage scenarios where Git might not be available or the directory is not a Git repository. For example:
EXECUTE_PROCESS( COMMAND git rev-parse HEAD OUTPUT_VARIABLE GIT_COMMIT_HASH ERROR_VARIABLE GIT_ERROR RESULT_VARIABLE GIT_RESULT OUTPUT_STRIP_TRAILING_WHITESPACE ) IF(NOT GIT_RESULT EQUAL 0) SET(GIT_COMMIT_HASH "unknown") MESSAGE(WARNING "Failed to get Git commit hash: ${GIT_ERROR}") ENDIF()
40-44: LGTM! Consider a more cross-platform approach.The implementation to capture the build date is correct for Unix-like systems. However, it may not work on all platforms (e.g., Windows).
Consider using CMake's built-in
${CMAKE_SYSTEM_NAME}variable to determine the operating system and use appropriate commands. For example:IF(CMAKE_SYSTEM_NAME STREQUAL "Windows") EXECUTE_PROCESS( COMMAND powershell -Command "Get-Date -Format 'yyyy-MM-dd HH:mm:ss K'" OUTPUT_VARIABLE BUILD_DATE OUTPUT_STRIP_TRAILING_WHITESPACE ) ELSE() EXECUTE_PROCESS( COMMAND date "+%Y-%m-%d %H:%M:%S %Z%z" OUTPUT_VARIABLE BUILD_DATE OUTPUT_STRIP_TRAILING_WHITESPACE ) ENDIF()This approach ensures compatibility across different operating systems.
46-47: LGTM! Consider adjusting log level.Logging the Git commit hash and build date is excellent for debugging and verification purposes.
Consider using
MESSAGE(DEBUG ...)instead ofMESSAGE(status ...)to reduce verbosity in non-debug builds:MESSAGE(DEBUG "GIT_COMMIT_HASH: ${GIT_COMMIT_HASH}") MESSAGE(DEBUG "GIT_COMMIT_DATE: ${BUILD_DATE}")This way, these messages will only appear when CMake is run in debug mode or with increased verbosity.
49-50: LGTM! Consider using modern CMake commands.Adding preprocessor definitions for the Git commit hash and build date is a good practice for embedding version information in the code.
Consider using the more modern
target_compile_definitions()command instead ofADD_DEFINITIONS(). This allows for better control over the scope of the definitions. For example:add_library(kiwi_version INTERFACE) target_compile_definitions(kiwi_version INTERFACE Kkiwi_GIT_COMMIT_ID="${GIT_COMMIT_HASH}" Kkiwi_BUILD_DATE="${BUILD_DATE}" ) # Later, when defining your main target: add_executable(kiwi_main ...) target_link_libraries(kiwi_main PRIVATE kiwi_version)This approach provides better encapsulation and allows for more flexible configuration management.
src/kiwi.cc (2)
29-31: Improved header organizationThe relocation of "praft/praft.h", "pstd/log.h", and "pstd/pstd_util.h" headers improves the organization of the include statements. This change enhances code readability without affecting functionality.
For consistency, consider grouping these headers with other project-specific headers (e.g., "client.h", "config.h", etc.) rather than placing them at the end of the include list.
Line range hint
1-385: Suggestions for overall code improvementWhile the changes made are improvements, there are some general suggestions for enhancing the codebase:
- Consider further modernizing the C++ code by consistently using C++ features and idioms throughout the file.
- Some functions (e.g.,
ParseArgs,Init) are quite long. Consider refactoring them into smaller, more focused functions to improve readability and maintainability.- The current error handling approach using logging and boolean returns could be enhanced. Consider implementing a more robust error handling mechanism, such as using exceptions or a custom error object, to provide more detailed error information and improve error recovery.
These suggestions are not directly related to the current changes but could be considered for future refactoring efforts.
src/storage/src/storage.cc (9)
Line range hint
1-16: Consider separating thread initialization from constructorThe constructor starts a background thread, which could lead to potential issues if the object construction fails. Consider moving the
StartBGThread()call to a separate initialization method that can be called after the object is fully constructed. This would allow for better error handling and resource management.Storage::Storage() { cursors_store_ = std::make_unique<LRUCache<std::string, std::string>>(); cursors_store_->SetCapacity(5000); - - Status s = StartBGThread(); - if (!s.ok()) { - ERROR("start bg thread failed, ", s.ToString()); - } } +Status Storage::Initialize() { + return StartBGThread(); +}
Line range hint
18-35: Improve thread safety and error handling in destructor
- Consider using
std::atomic<bool>foris_opened_to ensure thread-safe access.- The check for
is_opened_and the subsequent thread join could lead to a race condition. Consider settingis_opened_to false before signaling the thread to exit.- The error handling for
pthread_joinis good, but the code continues execution even if the join fails. Consider adding more robust error handling.+#include <atomic> class Storage { - bool is_opened_; + std::atomic<bool> is_opened_{false}; // ... other members ... }; Storage::~Storage() { INFO("Storage begin to clear storage!"); bg_tasks_should_exit_.store(true); bg_tasks_cond_var_.notify_one(); - if (is_opened_.load()) { + bool was_opened = is_opened_.exchange(false); + if (was_opened) { INFO("Storage begin to clear all instances!"); int ret = 0; if (ret = pthread_join(bg_tasks_thread_id_, nullptr); ret != 0) { ERROR("pthread_join failed with bgtask thread error : {}", ret); + // Consider more robust error handling here } insts_.clear(); } }
Line range hint
37-47: Enhance Close() method with synchronization and resource cleanupThe current
Close()method only sets flags without actually closing or cleaning up resources. Consider the following improvements:
- Add synchronization to ensure all ongoing operations are complete before closing.
- Implement actual resource cleanup, such as flushing data and closing file handles.
- Consider adding a timeout mechanism for waiting on ongoing operations.
Status Storage::Close() { if (!is_opened_.load()) { return Status::OK(); } - is_opened_.store(false); + + // Signal all operations to complete + std::unique_lock<std::mutex> lock(close_mutex_); + is_closing_.store(true); + + // Wait for ongoing operations to complete (with timeout) + if (!close_cv_.wait_for(lock, std::chrono::seconds(30), [this] { return ongoing_ops_ == 0; })) { + return Status::TimedOut("Timed out waiting for operations to complete"); + } + for (auto& inst : insts_) { - inst->SetNeedClose(true); + Status s = inst->Close(); + if (!s.ok()) { + return s; + } } + + is_opened_.store(false); return Status::OK(); }This refactored version adds synchronization, waits for ongoing operations to complete, and actually closes the instances.
Line range hint
49-83: Improve error handling and cleanup in Open() methodThe current implementation lacks proper error handling and cleanup in case of failures. Consider the following improvements:
- Add error handling for the
mkpathfunction call.- Implement proper cleanup if an instance fails to open.
- Use RAII techniques to ensure resource cleanup in case of exceptions.
Status Storage::Open(const StorageOptions& storage_options, const std::string& db_path) { - mkpath(db_path.c_str(), 0755); + if (mkpath(db_path.c_str(), 0755) != 0) { + return Status::IOError("Failed to create directory: " + db_path); + } db_instance_num_ = storage_options.db_instance_num; // Temporarily set to 100000 LogIndexAndSequenceCollector::max_gap_.store(storage_options.max_gap); storage_options.options.write_buffer_manager = std::make_shared<rocksdb::WriteBufferManager>(storage_options.mem_manager_size); + + std::vector<std::unique_ptr<Redis>> temp_insts; for (size_t index = 0; index < db_instance_num_; index++) { - insts_.emplace_back(std::make_unique<Redis>(this, index)); - Status s = insts_.back()->Open(storage_options, AppendSubDirectory(db_path, index)); + auto inst = std::make_unique<Redis>(this, index); + Status s = inst->Open(storage_options, AppendSubDirectory(db_path, index)); if (!s.ok()) { ERROR("open RocksDB{} failed {}", index, s.ToString()); - return Status::IOError(); + // Cleanup already opened instances + for (auto& opened_inst : temp_insts) { + opened_inst->Close(); + } + return s; } + temp_insts.push_back(std::move(inst)); INFO("open RocksDB{} success!", index); } + insts_ = std::move(temp_insts); slot_indexer_ = std::make_unique<SlotIndexer>(db_instance_num_); db_id_ = storage_options.db_id; is_opened_.store(true); return Status::OK(); }This refactored version adds proper error handling for directory creation, implements cleanup for partially opened instances in case of failure, and uses a temporary vector to ensure RAII-style cleanup if an exception occurs during the opening process.
Line range hint
85-100: Enhance CreateCheckpoint with error handling and consistency guaranteesWhile the current implementation efficiently creates checkpoints in parallel, it lacks error handling and doesn't guarantee consistency across all instances. Consider the following improvements:
- Implement error handling and result aggregation from the futures.
- Add a mechanism to ensure all checkpoints represent a consistent state across instances.
- Provide an option for atomic checkpoint creation.
-std::vector<std::future<Status>> Storage::CreateCheckpoint(const std::string& checkpoint_path) { +Status Storage::CreateCheckpoint(const std::string& checkpoint_path, bool atomic = false) { INFO("DB{} begin to generate a checkpoint to {}", db_id_, checkpoint_path); + if (atomic) { + // Implement a mechanism to pause writes across all instances + // This could involve acquiring a global write lock or using a distributed barrier + } + std::vector<std::future<Status>> result; result.reserve(db_instance_num_); for (int i = 0; i < db_instance_num_; ++i) { // In a new thread, create a checkpoint for the specified rocksdb i. auto res = std::async(std::launch::async, &Storage::CreateCheckpointInternal, this, checkpoint_path, i); result.push_back(std::move(res)); } + + // Wait for all futures and aggregate results + std::vector<Status> statuses; + for (auto& fut : result) { + statuses.push_back(fut.get()); + } + + if (atomic) { + // Release the global write lock or distributed barrier + } + + // Analyze statuses and return an appropriate result + for (const auto& status : statuses) { + if (!status.ok()) { + return status; + } + } + + return Status::OK(); }This refactored version adds an option for atomic checkpoint creation, implements error handling and result aggregation, and provides a more meaningful return value indicating the overall success or failure of the checkpoint operation.
Line range hint
102-153: Modernize file operations and add timeout mechanism in CreateCheckpointInternalThe current implementation uses C-style file operations and lacks a timeout mechanism for long-running operations. Consider the following improvements:
- Replace C-style file operations with modern C++ equivalents (std::filesystem).
- Add a timeout mechanism for checkpoint creation.
- Use RAII for resource management to ensure cleanup in all cases.
+#include <chrono> +#include <filesystem> Status Storage::CreateCheckpointInternal(const std::string& checkpoint_path, int index) { - auto source_dir = AppendSubDirectory(checkpoint_path, index); + std::filesystem::path source_dir = std::filesystem::path(checkpoint_path) / std::to_string(index); + std::filesystem::path tmp_dir = source_dir.string() + ".tmp"; - auto tmp_dir = source_dir + ".tmp"; // 1) Make sure the temporary directory does not exist - if (!pstd::DeleteDirIfExist(tmp_dir)) { - WARN("DB{}'s RocksDB {} delete directory fail!", db_id_, index); - return Status::IOError("DeleteDirIfExist() fail! dir_name : {} ", tmp_dir); + std::error_code ec; + if (std::filesystem::exists(tmp_dir) && !std::filesystem::remove_all(tmp_dir, ec)) { + WARN("DB{}'s RocksDB {} delete directory fail: {}", db_id_, index, ec.message()); + return Status::IOError("Failed to delete temporary directory: " + tmp_dir.string()); } // 2) Create checkpoint object of this RocksDB rocksdb::Checkpoint* checkpoint = nullptr; auto db = insts_[index]->GetDB(); rocksdb::Status s = rocksdb::Checkpoint::Create(db, &checkpoint); if (!s.ok()) { WARN("DB{}'s RocksDB {} create checkpoint object failed!. Error: ", db_id_, index, s.ToString()); return s; } // 3) Create a checkpoint std::unique_ptr<rocksdb::Checkpoint> checkpoint_guard(checkpoint); - s = checkpoint->CreateCheckpoint(tmp_dir, kFlush, nullptr); + auto future = std::async(std::launch::async, [&]() { + return checkpoint->CreateCheckpoint(tmp_dir.string(), kFlush, nullptr); + }); + + if (future.wait_for(std::chrono::minutes(5)) == std::future_status::timeout) { + WARN("DB{}'s RocksDB {} checkpoint creation timed out", db_id_, index); + return Status::TimedOut("Checkpoint creation timed out"); + } + + s = future.get(); if (!s.ok()) { WARN("DB{}'s RocksDB {} create checkpoint failed!. Error: {}", db_id_, index, s.ToString()); return s; } // 4) Make sure the source directory does not exist - if (!pstd::DeleteDirIfExist(source_dir)) { - WARN("DB{}'s RocksDB {} delete directory {} fail!", db_id_, index, source_dir); - if (!pstd::DeleteDirIfExist(tmp_dir)) { - WARN("DB{}'s RocksDB {} fail to delete the temporary directory {} ", db_id_, index, tmp_dir); + if (std::filesystem::exists(source_dir) && !std::filesystem::remove_all(source_dir, ec)) { + WARN("DB{}'s RocksDB {} delete directory {} fail: {}", db_id_, index, source_dir.string(), ec.message()); + if (!std::filesystem::remove_all(tmp_dir, ec)) { + WARN("DB{}'s RocksDB {} fail to delete the temporary directory {}: {}", db_id_, index, tmp_dir.string(), ec.message()); } - return Status::IOError("DeleteDirIfExist() fail! dir_name : {} ", source_dir); + return Status::IOError("Failed to delete source directory: " + source_dir.string()); } // 5) Rename the temporary directory to source directory - if (auto status = pstd::RenameFile(tmp_dir, source_dir); status != 0) { - WARN("DB{}'s RocksDB {} rename temporary directory {} to source directory {} fail!", db_id_, index, tmp_dir, - source_dir); - if (!pstd::DeleteDirIfExist(tmp_dir)) { - WARN("DB{}'s RocksDB {} fail to delete the rename failed directory {} ", db_id_, index, tmp_dir); + if (!std::filesystem::rename(tmp_dir, source_dir, ec)) { + WARN("DB{}'s RocksDB {} rename temporary directory {} to source directory {} fail: {}", + db_id_, index, tmp_dir.string(), source_dir.string(), ec.message()); + if (!std::filesystem::remove_all(tmp_dir, ec)) { + WARN("DB{}'s RocksDB {} fail to delete the rename failed directory {}: {}", + db_id_, index, tmp_dir.string(), ec.message()); } - return Status::IOError("Rename directory {} fail!", tmp_dir); + return Status::IOError("Failed to rename temporary directory: " + tmp_dir.string()); } - INFO("DB{}'s RocksDB {} create checkpoint {} success!", db_id_, index, source_dir); + INFO("DB{}'s RocksDB {} create checkpoint {} success!", db_id_, index, source_dir.string()); return Status::OK(); }This refactored version uses modern C++ file operations from std::filesystem, adds a timeout mechanism for checkpoint creation, and improves error handling and logging. It also uses RAII principles to ensure proper resource management.
Line range hint
155-170: Enhance LoadCheckpoint with error handling and consistency guaranteesWhile the current implementation efficiently loads checkpoints in parallel, it lacks error handling and doesn't guarantee consistency across all instances. Consider the following improvements:
- Implement error handling and result aggregation from the futures.
- Add a mechanism to ensure all checkpoints are loaded consistently across instances.
- Provide an option for atomic checkpoint loading.
-std::vector<std::future<Status>> Storage::LoadCheckpoint(const std::string& checkpoint_sub_path, - const std::string& db_sub_path) { +Status Storage::LoadCheckpoint(const std::string& checkpoint_sub_path, + const std::string& db_sub_path, + bool atomic = false) { INFO("DB{} begin to load a checkpoint from {} to {}", db_id_, checkpoint_sub_path, db_sub_path); + + if (atomic) { + // Implement a mechanism to pause all database operations across all instances + // This could involve acquiring a global lock or using a distributed barrier + } + std::vector<std::future<Status>> result; result.reserve(db_instance_num_); for (int i = 0; i < db_instance_num_; ++i) { // In a new thread, Load a checkpoint for the specified rocksdb i auto res = std::async(std::launch::async, &Storage::LoadCheckpointInternal, this, checkpoint_sub_path, db_sub_path, i); result.push_back(std::move(res)); } - return result; + + // Wait for all futures and aggregate results + std::vector<Status> statuses; + for (auto& fut : result) { + statuses.push_back(fut.get()); + } + + if (atomic) { + // Release the global lock or distributed barrier + } + + // Analyze statuses and return an appropriate result + for (const auto& status : statuses) { + if (!status.ok()) { + return status; + } + } + + return Status::OK(); }This refactored version adds an option for atomic checkpoint loading, implements error handling and result aggregation, and provides a more meaningful return value indicating the overall success or failure of the checkpoint loading operation. It also sets up a framework for ensuring consistency across all instances during the loading process.
Line range hint
172-217: Modernize file operations, add timeout, and improve error handling in LoadCheckpointInternalThe current implementation uses C-style file operations, lacks a timeout mechanism for long-running operations, and could benefit from improved error handling. Consider the following improvements:
- Replace C-style file operations with modern C++ equivalents (std::filesystem).
- Add a timeout mechanism for checkpoint loading.
- Improve error handling and recovery processes.
- Use RAII for resource management to ensure cleanup in all cases.
+#include <chrono> +#include <filesystem> Status Storage::LoadCheckpointInternal(const std::string& checkpoint_sub_path, const std::string& db_sub_path, int index) { - auto rocksdb_path = AppendSubDirectory(db_sub_path, index); // ./db/db_id/index - auto tmp_rocksdb_path = rocksdb_path + ".tmp"; // ./db/db_id/index.tmp + std::filesystem::path rocksdb_path = std::filesystem::path(db_sub_path) / std::to_string(index); + std::filesystem::path tmp_rocksdb_path = rocksdb_path.string() + ".tmp"; - auto source_dir = AppendSubDirectory(checkpoint_sub_path, index); + std::filesystem::path source_dir = std::filesystem::path(checkpoint_sub_path) / std::to_string(index); + // 1) Rename the original db to db.tmp, and only perform the maximum possible recovery of data // when loading the checkpoint fails. - if (auto status = pstd::RenameFile(rocksdb_path, tmp_rocksdb_path); status != 0) { - WARN("DB{}'s RocksDB {} rename db directory {} to temporary directory {} fail!", db_id_, index, rocksdb_path, - tmp_rocksdb_path); - return Status::IOError("Rename directory {} fail!", rocksdb_path); + std::error_code ec; + if (!std::filesystem::rename(rocksdb_path, tmp_rocksdb_path, ec)) { + WARN("DB{}'s RocksDB {} rename db directory {} to temporary directory {} fail: {}", + db_id_, index, rocksdb_path.string(), tmp_rocksdb_path.string(), ec.message()); + return Status::IOError("Failed to rename directory: " + rocksdb_path.string()); } // 2) Create a db directory to save the checkpoint. - if (0 != pstd::CreatePath(rocksdb_path)) { - pstd::RenameFile(tmp_rocksdb_path, rocksdb_path); - WARN("DB{}'s RocksDB {} load a checkpoint from {} fail!", db_id_, index, checkpoint_sub_path); - return Status::IOError("Create directory {} fail!", rocksdb_path); + if (!std::filesystem::create_directories(rocksdb_path, ec)) { + std::filesystem::rename(tmp_rocksdb_path, rocksdb_path, ec); + WARN("DB{}'s RocksDB {} load a checkpoint from {} fail: {}", + db_id_, index, checkpoint_sub_path, ec.message()); + return Status::IOError("Failed to create directory: " + rocksdb_path.string()); } - if (RecursiveLinkAndCopy(source_dir, rocksdb_path) != 0) { - pstd::DeleteDir(rocksdb_path); - pstd::RenameFile(tmp_rocksdb_path, rocksdb_path); - WARN("DB{}'s RocksDB {} load a checkpoint from {} fail!", db_id_, index, source_dir); - return Status::IOError("recursive link and copy directory {} fail!", rocksdb_path); + + // Add timeout mechanism for RecursiveLinkAndCopy + auto future = std::async(std::launch::async, [&]() { + return RecursiveLinkAndCopy(source_dir, rocksdb_path); + }); + + if (future.wait_for(std::chrono::minutes(30)) == std::future_status::timeout) { + std::filesystem::remove_all(rocksdb_path, ec); + std::filesystem::rename(tmp_rocksdb_path, rocksdb_path, ec); + WARN("DB{}'s RocksDB {} load a checkpoint from {} timed out", db_id_, index, source_dir.string()); + return Status::TimedOut("Checkpoint loading timed out"); + } + + if (future.get() != 0) { + std::filesystem::remove_all(rocksdb_path, ec); + std::filesystem::rename(tmp_rocksdb_path, rocksdb_path, ec); + WARN("DB{}'s RocksDB {} load a checkpoint from {} fail!", db_id_, index, source_dir.string()); + return Status::IOError("Failed to recursively link and copy directory: " + rocksdb_path.string()); } // 3) Destroy the db.tmp directory. if (auto s = rocksdb::DestroyDB(tmp_rocksdb_path, rocksdb::Options()); !s.ok()) { - WARN("Failure to destroy the old DB, path = {}", tmp_rocksdb_path); + WARN("Failure to destroy the old DB, path = {}: {}", tmp_rocksdb_path.string(), s.ToString()); + // Consider additional error handling or recovery steps here } return Status::OK(); }This refactored version uses modern C++ file operations from std::filesystem, adds a timeout mechanism for the recursive link and copy operation, improves error handling and logging, and uses RAII principles to ensure proper resource management. It also provides more detailed error messages and sets up a framework for potential additional recovery steps in case of failures.
Line range hint
219-229: Modernize thread creation and improve error handling in StartBGThreadThe current implementation uses low-level, platform-specific pthread_create. Consider the following improvements:
- Replace pthread_create with std::thread for better portability and exception handling.
- Implement a mechanism to ensure the thread is properly joined or detached.
- Improve error handling with more specific error codes.
+#include <thread> +#include <system_error> Status Storage::StartBGThread() { - int result = pthread_create(&bg_tasks_thread_id_, nullptr, StartBGThreadWrapper, this); - if (result != 0) { - char msg[128]; - snprintf(msg, sizeof(msg), "pthread create: %s", strerror(result)); - return Status::Corruption(msg); + try { + bg_tasks_thread_ = std::thread(StartBGThreadWrapper, this); + } catch (const std::system_error& e) { + return Status::Corruption("Failed to create background thread: " + std::string(e.what())); } return Status::OK(); } +Status Storage::StopBGThread() { + if (bg_tasks_thread_.joinable()) { + bg_tasks_should_exit_.store(true); + bg_tasks_cond_var_.notify_one(); + bg_tasks_thread_.join(); + } + return Status::OK(); +}This refactored version uses std::thread for better portability and exception handling. It also adds a StopBGThread method to ensure proper thread cleanup. The error handling is improved with more specific error messages. Remember to update the class definition to include the new std::thread member and the StopBGThread method.
src/replication.cc (4)
Line range hint
54-78: Logical Error:rdbFile Not Opened Before UseIn the
OnRdbSaveDone()method, thepstd::InputMemoryFile rdb;object is not opened before checkingrdb.IsOpen()or attempting to read from it. This meansrdb.IsOpen()will always returnfalse, triggering the error log and premature return. Additionally, attempting to read from an unopened file will lead to undefined behavior.To fix this issue, ensure that the
rdbfile is properly opened before use. Here's the proposed change:void PReplication::OnRdbSaveDone() { bgsaving_ = false; pstd::InputMemoryFile rdb; + if (!rdb.Open(g_config.rdbfullname.c_str())) { + ERROR("Cannot open RDB file: {}\n", g_config.rdbfullname); + return; // fatal error + } // send rdb to slaves that wait rdb end, set state for (auto& wptr : slaves_) { auto cli = wptr.lock(); if (!cli) { continue; } if (cli->GetSlaveInfo()->state == kPSlaveStateWaitBgsaveEnd) { cli->GetSlaveInfo()->state = kPSlaveStateOnline; - if (!rdb.IsOpen()) { - ERROR("can not open rdb when replication\n"); - return; // fatal error; - } std::size_t size = std::size_t(-1); const char* data = rdb.Read(size); // $file_len + filedata char tmp[32]; int n = snprintf(tmp, sizeof tmp - 1, "$%ld\r\n", (long)size); // evbuffer_iovec iovecs[] = { // {tmp, (size_t)(n)}, {const_cast<char*>(data), size}, {buffer_.ReadAddr(), buffer_.ReadableSize()}}; // cli->SendPacket(iovecs, sizeof(iovecs) / sizeof(iovecs[0])); INFO("Send to slave RDB of size {}, buffer {}", size, buffer_.ReadableSize()); } } buffer_.Clear(); }
Line range hint
85-87: Unreachable Code Due to Early Return inTryBgsave()The
TryBgsave()method contains an immediatereturn;statement at the beginning, which makes the rest of the function unreachable. This effectively disables the background save functionality.If disabling
TryBgsave()is intentional, consider adding a comment to explain the reasoning. If not, remove thereturn;statement to restore the original functionality.void PReplication::TryBgsave() { - return; if (IsBgsaving()) { return; } if (!HasAnyWaitingBgsave()) { return; } // Background save logic... }
Line range hint
85-98: Consistency Issue: Mixture of Comment StylesWithin the commented-out section of the
TryBgsave()method, there is inconsistent use of comment styles (//and/* ... */). Consistent commenting improves readability and maintainability.Consider standardizing the comment style throughout the codebase. For example, use
//for single-line comments and/* ... */for multi-line comments.
Line range hint
54-78: Potential Issue with Uninitialized VariablesizeIn the
OnRdbSaveDone()method,sizeis initialized tostd::size_t(-1), which sets it to the maximum value ofsize_t. This value is then used inrdb.Read(size). This could lead to unexpected behavior or overflow issues.Initialize
sizeappropriately after successfully opening the RDB file. Typically, you would get the actual size of the file.+ std::size_t size = rdb.GetFileSize(); - std::size_t size = std::size_t(-1); const char* data = rdb.Read(size);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 623a0868d48dcd26cbe9bf90ea6cada4034561c6 and 228da1c917fc3c223bb47232091a753fe88607f6.
📒 Files selected for processing (9)
- CMakeLists.txt (1 hunks)
- etc/script/build.sh (0 hunks)
- src/base_cmd.cc (1 hunks)
- src/cmd_thread_pool_worker.cc (1 hunks)
- src/config.h (1 hunks)
- src/kiwi.cc (1 hunks)
- src/pstd/memory_file.h (5 hunks)
- src/replication.cc (2 hunks)
- src/storage/src/storage.cc (1 hunks)
💤 Files with no reviewable changes (1)
- etc/script/build.sh
✅ Files skipped from review due to trivial changes (2)
- src/cmd_thread_pool_worker.cc
- src/config.h
🧰 Additional context used
🔇 Additional comments (12)
src/base_cmd.cc (2)
Line range hint
1-124: Clarify discrepancy between AI summary and visible codeThe AI-generated summary mentions a new constructor for
BaseCmdGroup:BaseCmdGroup::BaseCmdGroup(const std::string& name, uint32_t flag)However, this constructor is not visible in the provided code. Could you please clarify if this change was reverted, if it exists in a different part of the codebase, or if the AI summary might be incorrect?
To help clarify this discrepancy, please run the following script:
#!/bin/bash # Description: Search for BaseCmdGroup constructors in the codebase # Test: Look for BaseCmdGroup constructors rg --type cpp -A 3 'BaseCmdGroup::BaseCmdGroup' # Test: Check if there are any recent changes to BaseCmdGroup in git history git log -n 5 --pretty=format:"%h - %s" --grep="BaseCmdGroup"
19-19: Approve re-inclusion of "log.h" headerThe re-inclusion of "log.h" is appropriate, as it's likely needed for the DEBUG macro used in the
Executemethod. This change aligns with the PR objectives of fixing build information issues.To ensure this change doesn't introduce any unintended side effects, please run the following script:
✅ Verification successful
Approve re-inclusion of "log.h" header
The inclusion of "log.h" is confirmed to be utilized within the file, ensuring that the DEBUG macro functions correctly. No unused headers were detected. This change aligns with the PR objectives of fixing build information issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of log-related functions or macros in the file # Test: Search for log-related function calls or macro usages rg --type cpp -i '\b(log|debug|info|warn|error|fatal)\b' src/base_cmd.cc # Test: Check if there are any unused includes rg --type cpp -e '^#include' src/base_cmd.cc | cut -d'"' -f2 | cut -d'<' -f2 | cut -d'>' -f1 | while read header; do if ! rg --type cpp -q "$(basename "$header" .h)" src/base_cmd.cc; then echo "Potentially unused header: $header" fi doneLength of output: 660
Script:
#!/bin/bash # Description: Verify the usage of log-related functions or macros in the file # Test: Search for log-related function calls or macro usages rg --type cpp -i '\b(log|debug|info|warn|error|fatal)\b' src/base_cmd.cc # Test: Check if there are any unused includes rg --type cpp '^#include' src/base_cmd.cc | \ sed -E 's/#include\s+"([^"]+)".*/\1/' | \ while read -r header; do basename=$(basename "$header" .h) if ! rg --type cpp -q "\b$basename\b" src/base_cmd.cc; then echo "Potentially unused header: $header" fi doneLength of output: 1094
src/pstd/memory_file.h (4)
109-109: Improved destructor documentation.The updated comment for the destructor provides more clarity on its behavior, especially in the case of an invalid file descriptor. This improvement in documentation is beneficial for developers using this class.
134-134: ImprovedClose()method documentation.The updated comment for the
Close()method is more concise and clearly states its purpose. This change enhances the overall documentation quality of the class.
143-146: ImprovedTruncate()method documentation.The updated comment for the
Truncate()method now provides more comprehensive information about its functionality. It clearly states that the method not only adjusts the file size but also rebuilds the mapping relationship. This additional detail is valuable for developers using this class, as it helps them understand the full impact of calling this method.
157-159: ImprovedWrite()method documentation.The updated comment for the
Write()method is more concise and accurately describes its core functionality. It now clearly states that the method writes content to the buffer and adjusts the offset, which aligns well with the method's actual behavior. This improvement in documentation enhances the overall clarity of the class interface.CMakeLists.txt (1)
34-50: Overall assessment: Changes effectively address the PR objectives.The modifications successfully implement the capturing and embedding of Git commit hash and build date information into the CMake configuration. This addresses the issue mentioned in the PR objectives where the build information was not correctly added to the code.
Key points:
- Git commit hash is now correctly captured and made available.
- Build date is captured, including timezone information.
- Both pieces of information are logged during the CMake configuration process.
- Preprocessor definitions are added to make this information available in the code.
These changes should resolve the issue of incorrect git information and build date in the code. However, consider implementing the suggested improvements for better cross-platform compatibility, error handling, and adherence to modern CMake practices.
src/kiwi.cc (1)
20-20: Good practice: Using C++ standard library headerThe change from
<time.h>to<ctime>is a good practice in C++. It uses the C++ standard library header instead of the C header, which is more appropriate for a C++ codebase and improves consistency.src/storage/src/storage.cc (1)
Line range hint
231-243: Improve task management and memory safety in AddBGTaskThe current implementation has potential issues with task management and memory safety. Consider the following improvements:
- Use
src/replication.cc (3)
17-17: Inclusion of "log.h" for Logging FunctionalityIncluding
"log.h"enables logging functions likeINFOandERRORused throughout the file. This inclusion is appropriate and necessary for the logging enhancements made in the code.
Line range hint
210-219: Functionality Disabled inSaveTmpRdb()The entire body of the
SaveTmpRdb()method is commented out, and the function immediately returns. This means that saving of temporary RDB data from the master is currently disabled, which could impact replication synchronization.Is disabling this functionality intentional? If so, consider removing the method entirely to clean up the code. If not, uncomment the code to restore the functionality and ensure proper synchronization with the master.
Line range hint
192-198: Confirmation Needed: Correct Usage ofon_new_connLambda FunctionIn the
Cron()method, theon_new_connlambda function is defined and passed toTCPConnect(). The lambda captures the connection ID, client, and address, and then callsg_kiwi->OnNewConnection()ifg_kiwiis valid.Ensure that
on_new_conncorrectly handles new connections and thatg_kiwi->OnNewConnection()is the appropriate method to call in this context.
上次修改编译宏时,没有验证对应的信息是否正确的添加到代码中,导致代码中的 git信息和构建日期这些信息都是错的, 非常抱歉, 没有验证就提交代码了
这是现在的效果
Summary by CodeRabbit
New Features
Bug Fixes
Refactor