fix: use master's wall time(physical time) as ttl in binlog #2699
Conversation
WalkthroughThis update introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PikaClientConn
participant PikaServer
participant LogicTime
Client ->> PikaClientConn: Execute Command
PikaClientConn ->> PikaServer: Check Read-Only Status
PikaServer -->> PikaClientConn: Read-Only Status
alt Read-Only
PikaClientConn ->> LogicTime: Set ProtectionMode(true)
PikaClientConn ->> LogicTime: Now()
LogicTime -->> PikaClientConn: Current Time
PikaClientConn ->> LogicTime: Set ProtectionMode(false)
else
PikaClientConn ->> LogicTime: Now()
LogicTime -->> PikaClientConn: Current Time
end
PikaClientConn -->> Client: Command Result
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/pika_binlog_transverter.cc (2 hunks)
- src/storage/include/storage/storage_time.h (1 hunks)
- src/storage/src/base_filter.h (1 hunks)
- src/storage/src/base_value_format.h (2 hunks)
- src/storage/src/storage_time.cc (1 hunks)
- tools/binlog_sender/binlog_transverter.cc (2 hunks)
- tools/pika-port/pika_port_3/binlog_transverter.cc (2 hunks)
Additional comments not posted (10)
src/storage/src/storage_time.cc (2)
24-26: LGTM! Simple and effective update oflogic_time_.
28-30: LGTM! Proper use ofstd::atomicfor thread-safe read.src/storage/include/storage/storage_time.h (1)
38-39: LGTM! Proper declaration of the global unique pointer.tools/binlog_sender/binlog_transverter.cc (1)
76-76: Verify the update of logical time occurs at the correct point in theBinlogDecodeprocess.Ensure that updating the logical time before parsing the
binlog_strdoes not introduce any side effects or race conditions.tools/pika-port/pika_port_3/binlog_transverter.cc (1)
81-81: Ensure the logical time update is correctly sequenced in thePortBinlogDecodeprocess.Check that the update to the logical time before processing the
binlog_strdoes not cause unintended consequences or timing issues.Verification successful
The logical time update in the
PortBinlogDecodefunction is placed before extracting theexec_time_from thebinlog_str. This ensures that the logical time is updated with the correct execution time from thebinlog_item.
- The logical time update is correctly sequenced within the
PortBinlogDecodefunction.- The function is used in
master_conn.ccwithin a conditional block that handleskTypePortBinlog.No issues were found with the placement of the logical time update.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logical time update is correctly placed in the `PortBinlogDecode` function. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type cpp $'PortBinlogDecode'Length of output: 456
Script:
#!/bin/bash # Extract the implementation of PortBinlogDecode from binlog_transverter.cc ast-grep --lang cpp --pattern $'bool PortBinlogTransverter::PortBinlogDecode($_, $_, $_) { $$$ }' tools/pika-port/pika_port_3/binlog_transverter.cc # Extract the context of PortBinlogDecode usage in master_conn.cc rg -A 10 -B 10 'PortBinlogDecode' tools/pika-port/pika_port_3/master_conn.ccLength of output: 984
Script:
#!/bin/bash # Extract the full implementation of PortBinlogDecode from binlog_transverter.cc ast-grep --lang cpp --pattern $'bool PortBinlogTransverter::PortBinlogDecode(PortBinlogType type, const std::string& binlog, PortBinlogItem* binlog_item) { $$$ }' tools/pika-port/pika_port_3/binlog_transverter.ccLength of output: 215
Script:
#!/bin/bash # Search for the definition of PortBinlogDecode in binlog_transverter.cc and extract its implementation rg -A 20 'bool PortBinlogTransverter::PortBinlogDecode' tools/pika-port/pika_port_3/binlog_transverter.ccLength of output: 1156
src/storage/src/base_value_format.h (2)
139-139: Updated logic to use logical time instead of system time.This change aligns with the PR's objective to use logical time for operations, enhancing consistency across different components.
18-18: Ensure the new header inclusion is necessary and correctly placed.Verification successful
The new header inclusion
#include "storage/storage_time.h"is necessary and correctly placed, asg_storage_logictimeis used in the file.
g_storage_logictimeis referenced insrc/storage/src/base_value_format.h.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new header is used in this file. # Test: Search for usage of `g_storage_logictime` in this file. Expect: At least one usage. rg --type cpp 'g_storage_logictime' src/storage/src/base_value_format.hLength of output: 123
src/pika_binlog_transverter.cc (2)
88-88: UpdatedBinlogDecodeto use logical time for updating execution time.This change ensures that the binlog processing logic is consistent with the new logical time system, potentially improving the accuracy of time-dependent operations.
17-17: Ensure the new header inclusion is necessary and correctly placed.Verification successful
The new header inclusion
#include "storage/storage_time.h"is necessary and correctly placed, as theg_storage_logictimefrom this header is used in the file.
src/pika_binlog_transverter.cc:storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time());Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new header is used in this file. # Test: Search for usage of `g_storage_logictime` in this file. Expect: At least one usage. rg --type cpp 'g_storage_logictime' src/pika_binlog_transverter.ccLength of output: 141
src/storage/src/base_filter.h (1)
30-31: Updated logic to use logical time instead of system time.This change ensures that the compaction filter logic uses the new logical time system, which can improve the accuracy and consistency of time-dependent operations.
| class LogicTime { | ||
| public: | ||
| // Constructor | ||
| LogicTime() : logic_time_(0), protection_mode_(false) {} | ||
|
|
||
| // Get the current logical time or the system time | ||
| int64_t Now(); | ||
|
|
||
| // Update the logical time | ||
| void UpdateLogicTime(int64_t time); | ||
|
|
||
| // Get the protection mode status | ||
| bool ProtectionMode(); | ||
|
|
||
| // Set the protection mode | ||
| void SetProtectionMode(bool on); | ||
|
|
||
| private: | ||
| std::atomic<int64_t> logic_time_; // Logical time value | ||
| std::atomic<bool> protection_mode_; // Protection mode flag | ||
| }; |
There was a problem hiding this comment.
Ensure encapsulation and thread safety in LogicTime.
The LogicTime class uses std::atomic for logic_time_ and protection_mode_, which is good for thread safety. However, consider making the atomic variables private and providing thread-safe public methods to manipulate these values to ensure encapsulation and safety.
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/pika_client_conn.cc (2 hunks)
- src/storage/src/storage_time.cc (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/storage/src/storage_time.cc
Additional comments not posted (1)
src/pika_client_conn.cc (1)
189-189: Correctly resets the protection mode after command execution.
src/pika_client_conn.cc
Outdated
| @@ -164,6 +164,7 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st | |||
| return c_ptr; | |||
| } | |||
| if (g_pika_server->readonly(current_db_) && opt != kCmdNameExec) { | |||
| storage::g_storage_logictime->SetProtectionMode(true); | |||
There was a problem hiding this comment.
Refactor to improve clarity in setting protection mode.
- storage::g_storage_logictime->SetProtectionMode(true);
- c_ptr->res().SetRes(CmdRes::kErrOther, "READONLY You can't write against a read only replica.");
+ if (g_pika_server->readonly(current_db_)) {
+ storage::g_storage_logictime->SetProtectionMode(true);
+ c_ptr->res().SetRes(CmdRes::kErrOther, "READONLY You can't write against a read only replica.");
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| storage::g_storage_logictime->SetProtectionMode(true); | |
| if (g_pika_server->readonly(current_db_)) { | |
| storage::g_storage_logictime->SetProtectionMode(true); | |
| c_ptr->res().SetRes(CmdRes::kErrOther, "READONLY You can't write against a read only replica."); | |
| } |
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| #include "binlog_transverter.h" | |||
| #include <glog/logging.h> | |||
| #include "storage/storage_time.h" | |||
There was a problem hiding this comment.
这些工具就没必要改了吧，他们适配的目标是 3.0，不是 3.5 or 4.0
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/pika_client_conn.cc (2 hunks)
- src/storage/src/storage_time.cc (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/pika_client_conn.cc
- src/storage/src/storage_time.cc
AlexStocks
changed the title
AlexStocks
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/pika_client_conn.cc (1 hunks)
- src/storage/include/storage/storage_time.h (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/pika_client_conn.cc
- src/storage/include/storage/storage_time.h
|
cjg 继续和邓昊 review |
cjg continues to review with Deng Hao |
chejinge
force-pushed
the
ttl_logic_time
branch
2 times, most recently
from
8824677 to
996feda
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/pika_binlog_transverter.cc (3 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/storage/src/base_filter.h (1 hunks)
- src/storage/src/base_value_format.h (2 hunks)
- src/storage/src/storage_time.cc (1 hunks)
Additional comments not posted (3)
src/storage/src/base_value_format.h (1)
18-18: Include statement added forstorage/storage_time.h.The inclusion of
storage/storage_time.his necessary for accessing theg_storage_logictimefunctionalities used in this file.src/pika_binlog_transverter.cc (1)
17-17: Include statement added forstorage/storage_time.h.The inclusion of
storage/storage_time.his necessary for accessing theg_storage_logictimefunctionalities used in the binlog decoding process.src/storage/src/base_filter.h (1)
31-33: Use ofg_storage_logictimeinFiltermethod.Proper use of
g_storage_logictimeto fetch the current time based on the protection mode. This ensures correct time comparisons during the compaction filtering process.
src/pika_client_conn.cc
Outdated
| if (g_pika_server->readonly(current_db_)) { | ||
| storage::g_storage_logictime->SetProtectionMode(true); | ||
| } | ||
| storage::g_storage_logictime->SetProtectionMode(false); |
There was a problem hiding this comment.
Refactor the protection mode setting to prevent logical errors.
- if (g_pika_server->readonly(current_db_)) {
- storage::g_storage_logictime->SetProtectionMode(true);
- }
- storage::g_storage_logictime->SetProtectionMode(false);
+ bool is_readonly = g_pika_server->readonly(current_db_);
+ storage::g_storage_logictime->SetProtectionMode(is_readonly);This change ensures that the protection mode is set based on the current database's read-only status in a single operation, preventing any logical inconsistencies.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (g_pika_server->readonly(current_db_)) { | |
| storage::g_storage_logictime->SetProtectionMode(true); | |
| } | |
| storage::g_storage_logictime->SetProtectionMode(false); | |
| bool is_readonly = g_pika_server->readonly(current_db_); | |
| storage::g_storage_logictime->SetProtectionMode(is_readonly); |
| int64_t LogicTime::Now() { | ||
| if (protection_mode_) { | ||
| return logic_time_; | ||
| } | ||
| int64_t t; | ||
| rocksdb::Env::Default()->GetCurrentTime(&t); | ||
| return t; | ||
| } |
There was a problem hiding this comment.
Ensure thread safety when accessing logic_time_ outside of protection_mode_.
Consider using atomic operations or mutexes to ensure thread safety for logic_time_ access in non-protection mode environments.
| void LogicTime::SetProtectionMode(bool on) { | ||
| protection_mode_ = on; | ||
| LOG(INFO) << "Set protection mode to: " << on; | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for logging failures.
Implement error handling or fallback mechanisms for logging within SetProtectionMode to enhance reliability in case of logging system failures or initialization issues.
src/storage/src/base_value_format.h
Outdated
| if (g_storage_logictime->ProtectionMode()){ | ||
| unix_time = g_storage_logictime->Now(); | ||
| } | ||
| unix_time = g_storage_logictime->Now(); |
There was a problem hiding this comment.
Redundant call to Now() in IsStale method.
- unix_time = g_storage_logictime->Now();
+ // Removed redundant line: unix_time is already set in the preceding lines.Remove the redundant call to g_storage_logictime->Now() to optimize performance and avoid unnecessary logical time fetches.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (g_storage_logictime->ProtectionMode()){ | |
| unix_time = g_storage_logictime->Now(); | |
| } | |
| unix_time = g_storage_logictime->Now(); | |
| if (g_storage_logictime->ProtectionMode()){ | |
| unix_time = g_storage_logictime->Now(); | |
| } |
src/pika_binlog_transverter.cc
Outdated
| @@ -167,6 +169,7 @@ bool PikaBinlogTransverter::BinlogItemWithoutContentDecode(BinlogType type, cons | |||
| LOG(ERROR) << "Binlog Item type error, expect type:" << type << " actualy type: " << binlog_type; | |||
| return false; | |||
| } | |||
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); | |||
There was a problem hiding this comment.
Ensure logical time is updated with correct execution time.
- storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time());
+ pstd::GetFixed32(&binlog_str, &binlog_item->exec_time_);
+ storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time());Similar to the previous comment, update the execution time from binlog_str before updating the logical time to prevent using an outdated value.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); | |
| pstd::GetFixed32(&binlog_str, &binlog_item->exec_time_); | |
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); |
src/pika_binlog_transverter.cc
Outdated
| @@ -84,6 +85,7 @@ | |||
| LOG(ERROR) << "Binlog Item type error, expect type:" << type << " actualy type: " << binlog_type; | |||
| return false; | |||
| } | |||
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); | |||
There was a problem hiding this comment.
Update logical time before fetching execution time.
- storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time());
+ pstd::GetFixed32(&binlog_str, &binlog_item->exec_time_);
+ storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time());Update the execution time from binlog_str before calling UpdateLogicTime to ensure the logic time is updated with the correct value.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); | |
| pstd::GetFixed32(&binlog_str, &binlog_item->exec_time_); | |
| storage::g_storage_logictime->UpdateLogicTime(binlog_item->exec_time()); |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/pika_binlog_transverter.cc (3 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/storage/include/storage/storage_time.h (1 hunks)
- src/storage/src/base_filter.h (1 hunks)
- src/storage/src/base_value_format.h (2 hunks)
- src/storage/src/storage_time.cc (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/pika_binlog_transverter.cc
- src/pika_client_conn.cc
- src/storage/src/base_filter.h
- src/storage/src/base_value_format.h
- src/storage/src/storage_time.cc
Additional comments not posted (4)
src/storage/include/storage/storage_time.h (4)
6-12: Header guards and necessary library inclusions are correctly implemented.
16-19: Initialization of atomic members in the constructor ensures thread safety from the start.
38-39: Ensureg_storage_logictimeis initialized correctly to avoid null pointer dereferences.Verification successful
The initialization of
g_storage_logictimeis correctly found insrc/storage/src/storage_time.ccwith the statementstd::unique_ptr<LogicTime> g_storage_logictime = std::make_unique<LogicTime>();. This ensures that the pointer is properly initialized, preventing null pointer dereferences.
src/storage/src/storage_time.cc:std::unique_ptr<LogicTime> g_storage_logictime = std::make_unique<LogicTime>();Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `g_storage_logictime`. # Test: Search for the initialization in the project. rg --type cpp $'std::unique_ptr<LogicTime> g_storage_logictime = std::make_unique<LogicTime>();'Length of output: 97
Script:
#!/bin/bash # Description: Verify the initialization of `g_storage_logictime` in various forms. # Test: Search for the initialization using different possible forms. rg --type cpp 'g_storage_logictime = std::make_unique<LogicTime>();' rg --type cpp 'g_storage_logictime = new LogicTime' rg --type cpp 'g_storage_logictime = std::make_shared<LogicTime>();' # Test: Search for any function that might initialize `g_storage_logictime`. rg --type cpp 'g_storage_logictime ='Length of output: 347
21-31: Ensure that the implementations of these methods handle atomic operations safely and efficiently.Verification successful
The implementations of the methods
Now,UpdateLogicTime,ProtectionMode, andSetProtectionModein theLogicTimeclass have been located and reviewed. The use ofstd::atomicforlogic_time_andprotection_mode_ensures that the operations on these variables are thread-safe and efficient.
- The
Nowmethod returns the current logical time if protection mode is enabled; otherwise, it retrieves the system time.- The
UpdateLogicTimemethod updates the logical time only if protection mode is enabled.- The
ProtectionModemethod simply returns the current state of the protection mode.- The
SetProtectionModemethod sets the protection mode and logs the change.These implementations correctly handle atomic operations and maintain thread safety.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of methods in LogicTime class. # Test: Search for method implementations in the project. ast-grep --lang cpp --pattern $'class LogicTime { $$$ Now($_) { $$$ } UpdateLogicTime($_) { $$$ } ProtectionMode() { $$$ } SetProtectionMode($_) { $$$ } $$$ }'Length of output: 543
Script:
#!/bin/bash # Description: Locate the LogicTime class definition and its method implementations. # Search for the LogicTime class definition in the project. rg -t cpp 'class LogicTime' -A 20 # Search for the implementations of the methods in the LogicTime class. rg -t cpp 'int64_t LogicTime::Now|void LogicTime::UpdateLogicTime|bool LogicTime::ProtectionMode|void LogicTime::SetProtectionMode' -A 10Length of output: 2897
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/pika_binlog_transverter.cc (3 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/storage/include/storage/storage_time.h (1 hunks)
- src/storage/src/base_filter.h (1 hunks)
- src/storage/src/base_value_format.h (4 hunks)
- src/storage/src/storage_time.cc (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/pika_binlog_transverter.cc
- src/pika_client_conn.cc
- src/storage/include/storage/storage_time.h
- src/storage/src/base_filter.h
- src/storage/src/base_value_format.h
- src/storage/src/storage_time.cc
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/storage/src/base_value_format.h (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/storage/src/base_value_format.h
|
这个暂时不修复 |
This will not be fixed for now |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
从是不会写的 感觉一直都是保护模式？