Skip to content
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.

Sign up for GitHub

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

fix slave's binlog offset greater than master bug #1682

Merged
merged 10 commits into from
Jul 9, 2023
Merged

fix slave's binlog offset greater than master bug #1682

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe29ed9
fix slave's binlog offset greater than master bug
Jul 4, 2023
049c634
FIX
Jul 4, 2023
30233d2
FIX
Jul 4, 2023
1b94dd8
FIX
Jul 4, 2023
606eca3
fix
Jul 5, 2023
9af8564
fix
Jul 5, 2023
24a0989
merge code
Jul 5, 2023
d9e0ca5
merge code
Jul 5, 2023
56a1e18
fix
Jul 6, 2023
6d678c3
add intrgration test for slot migrate key
luky116 Jul 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions codis/pkg/proxy/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
package proxy

import (
"sync"
"unsafe"

"pika/codis/v2/pkg/proxy/redis"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below is a code review of the provided code patch:

  • The import statement for the "sync" package has an indentation inconsistency. It should be indented with a tab or spaces to match the other imports.
  • The comment lines at the beginning of the file look incorrect. They should start with // instead of @@.
  • There is an unused import for "pika/codis/v2/pkg/proxy/redis". If it is not required, it can be safely removed.

Improvement suggestions:

  • It's important to provide more context and details about the code being reviewed in order to identify potential bug risks or improvements accurately.
  • Include the entire code snippet or function if possible, so that I can analyze it more comprehensively.

Please provide more specific information or code if you'd like a more detailed review or have any particular concerns.

"pika/codis/v2/pkg/utils/sync2/atomic2"
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, it seems that some import statements have been removed: "sync" and "unsafe". It's difficult to perform a comprehensive code review without the full context of the codebase, but here are a few general suggestions:

  1. Check if removing the "sync" and "unsafe" packages was intentional. These packages provide synchronization primitives and low-level memory access, respectively. Make sure their removal does not introduce any issues or break functionality.

  2. Review the changes made to the "import" block. Verify that the remaining imports are still necessary and relevant.

  3. Consider adding comments or documentation to explain the purpose and functionality of the package and individual functions, especially if they are non-trivial.

  4. Pay attention to error handling. Ensure that appropriate error checks and handling are implemented throughout the codebase.

  5. It's good practice to follow consistent formatting and coding conventions, such as indentation, naming conventions, and code organization, to improve code readability and maintainability.

  6. Review any logic or algorithm changes introduced in the patch. Verify that they are correct and handle all possible edge cases.

Remember, without more information about the surrounding code, it's challenging to provide specific recommendations. It's always useful to test the modified code thoroughly to identify any potential bugs or performance issues.

Expand Down
3 changes: 2 additions & 1 deletion include/pika_slot_command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

const std::string SlotKeyPrefix = "_internal:slotkey:4migrate:";
const std::string SlotTagPrefix = "_internal:slottag:4migrate:";
const size_t KSlotKeyPrefixLen = 27;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

别这么写

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

金鸽的意思是，把 size_t 改为int，大家评估下

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不是，27怎么来的？

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不是，27怎么来的？

SlotTagPrefix 字符串的长度

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不是，27怎么来的？

优化了下，直接使用 length


const size_t MaxKeySendSize = 10 * 1024;

extern uint32_t crc32tab[256];
Expand All @@ -32,7 +34,6 @@ std::string GetSlotKey(int slot);
std::string GetSlotsTagKey(uint32_t crc);
int GetSlotsID(const std::string &str, uint32_t *pcrc, int *phastag);
void RemSlotKeyByType(const std::string &type, const std::string &key, const std::shared_ptr<Slot>& slot);
void WriteSAddToBinlog(const std::string &key, const std::string &value, const std::shared_ptr<Slot>& slot);

class PikaMigrate {
public:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code patch you provided, here's a brief code review:

  1. It seems like kSlotKeyPrefixLen is introduced as a constant to store the length of SlotKeyPrefix.

  2. WriteSAddToBinlog() function has been removed or commented out. Ensure that this change doesn't affect the intended functionality. Verify if this function is no longer needed or if it should be replaced with an alternative implementation.

Apart from these specific changes, without the context of the entire codebase and its purpose, it's challenging to provide further bug risks or improvement suggestions. A comprehensive code review typically involves examining the logic, error handling, performance optimizations, coding conventions, and adherence to best practices. Consider providing more details or sharing the rest of the code if you require a more thorough review.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions for code review:

  1. Inconsistent formatting: The indentation of the code seems to be inconsistent. It's important to maintain a consistent coding style for better readability.

  2. Magic numbers: The value "27" appears in the code as the length of SlotKeyPrefix. Consider defining it as a constant, similar to kSlotKeyPrefixLen, to improve code clarity.

  3. Unused function: The function WriteSAddToBinlog is declared but not defined or used in the provided code patch. Make sure it is necessary and remove it if it's not needed.

  4. Missing function details: It would be helpful to see the implementation and usage of the functions GetSlotKey(), GetSlotsTagKey(), GetSlotsID(), RemSlotKeyByType(), and the class PikaMigrate to perform a more comprehensive code review.

  5. Commented out code: There doesn't seem to be any commented-out code in the provided patch, but it is good practice to remove any commented-out code from the final codebase.

Note: Without the complete code context and implementation details, it may be challenging to identify potential bugs or provide further improvement suggestions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the complete context and understanding of the codebase, it is challenging to provide a thorough code review. However, based on the provided code patch, here are some observations:

  1. Unused Function: The function WriteSAddToBinlog seems to be declared but not used in the code patch. If it is not necessary, consider removing it or verifying if it serves any purpose.

  2. Magic Numbers: The value 10 * 1024 appears as MaxKeySendSize. Consider using named constants or variables with descriptive names instead of "magic numbers" to improve code readability and maintainability.

  3. Style Consistency: Ensure consistent code formatting and indentation throughout the codebase for better readability and maintainability.

Apart from these specific observations, perform a thorough testing of the modified code to check for bug risks or potential improvements. Additionally, consider including comments that explain the purpose and functionality of key functions or blocks of code to aid future maintenance.

Expand Down
6 changes: 6 additions & 0 deletions src/pika_consensus.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ Status ConsensusCoordinator::Reset(const LogOffset& offset) {

Status ConsensusCoordinator::ProposeLog(const std::shared_ptr<Cmd>& cmd_ptr, std::shared_ptr<PikaClientConn> conn_ptr,
std::shared_ptr<std::string> resp_ptr) {
std::vector<std::string> keys = cmd_ptr->current_key();
// slotkey shouldn't add binlog
if (cmd_ptr->name() == kCmdNameSAdd && !keys.empty() && keys[0].compare(0, KSlotKeyPrefixLen, SlotKeyPrefix) == 0) {
return Status::OK();
}

LogOffset log_offset;

stable_logger_->Logger()->Lock();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch you provided seems fine. Here are a few observations and suggestions:

  1. The added code segment checks if the command name is "SAdd" and if the first key in the command starts with either "SlotKeyPrefix" or "SlotTagPrefix". If these conditions are met, it returns Status::OK() without further processing. This logic assumes that kCmdNameSAdd, KSlotKeyPrefixLen, and SlotKeyPrefix/SlotTagPrefix are correctly defined and initialized elsewhere in the code.

  2. One improvement suggestion for the added code segment is to use std::string::starts_with function (introduced in C++20) instead of the compare() method. It provides a cleaner and more expressive way to check if a string starts with a specific prefix.

Example:

if (cmd_ptr->name() == kCmdNameSAdd && !keys.empty() &&
    (keys[0].starts_with(SlotKeyPrefix) || keys[0].starts_with(SlotTagPrefix))) {
  return Status::OK();
}

  1. Make sure that the LogOffset log_offset variable is being used correctly after this code block. It seems it might be used later, so double-check its usage in the surrounding code for correctness.

  2. Consider adding comments to describe the purpose and rationale of the added code segment. Comments can improve code clarity and make it easier for future maintainers to understand the code's intent.

Remember to thoroughly test the code with various scenarios to ensure it functions as intended and doesn't introduce any unintended side effects.

Expand Down
4 changes: 2 additions & 2 deletions src/pika_repl_server_conn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,8 @@ bool PikaReplServerConn::TrySyncOffsetCheck(const std::shared_ptr<SyncMasterSlot
(boffset.filenum == slave_boffset.filenum() && boffset.offset < slave_boffset.offset())) {
try_sync_response->set_reply_code(InnerMessage::InnerResponse::TrySync::kSyncPointLarger);
LOG(WARNING) << "Slave offset is larger than mine, Slave ip: " << node.ip() << ", Slave port: " << node.port()
<< ", Slot: " << slot_name << ", filenum: " << slave_boffset.filenum()
<< ", pro_offset_: " << slave_boffset.offset();
<< ", Slot: " << slot_name << ", slave filenum: " << slave_boffset.filenum()
<< ", slave pro_offset_: " << slave_boffset.offset() << ", local filenum: " << boffset.filenum << ", local pro_offset_: " << boffset.offset;
return false;
}

Expand Down
19 changes: 0 additions & 19 deletions src/pika_slot_command.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,6 @@ void AddSlotKey(const std::string type, const std::string key, const std::shared
LOG(ERROR) << "sadd key[" << key << "] to slotKey[" << slot_key << "] failed, error: " << s.ToString();
return;
}
WriteSAddToBinlog(slot_key, members.front(), slot);

// if res == 0, indicate the key is existed; may return,
// prevent write slot_key success, but write tag_key failed, so always write tag_key
Expand All @@ -827,24 +826,6 @@ void AddSlotKey(const std::string type, const std::string key, const std::shared
LOG(ERROR) << "sadd key[" << key << "] to tagKey[" << tag_key << "] failed, error: " << s.ToString();
return;
}
WriteSAddToBinlog(tag_key, members.front(), slot);
}
}

// write sadd key to binlog for slave
void WriteSAddToBinlog(const std::string &key, const std::string &value, const std::shared_ptr<Slot>& slot) {
std::shared_ptr<Cmd> cmd_ptr = g_pika_cmd_table_manager->GetCmd("sadd");
std::unique_ptr<PikaCmdArgsType> args = std::unique_ptr<PikaCmdArgsType>(new PikaCmdArgsType());
args->push_back("SADD");
args->push_back(key);
args->push_back(value);
cmd_ptr->Initial(*args, slot->GetDBName());

std::shared_ptr<SyncMasterSlot> sync_slot =
g_pika_rm->GetSyncMasterSlotByName(SlotInfo(slot->GetDBName(), slot->GetSlotID()));
Status s = sync_slot->ConsensusProposeLog(cmd_ptr);
if (!s.ok()) {
LOG(ERROR) << "write sadd key to binlog failed, key: " << key;
}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, here are some observations and suggestions:

  1. It appears that the function WriteSAddToBinlog has been removed from the code. If this is intentional and no longer required, then the removal is fine. Otherwise, double-check if it should be kept or if there's an alternative implementation planned.

  2. There doesn't seem to be any bug risk in the modified code section since you have removed the WriteSAddToBinlog function call along with its log output. However, without the full context of the codebase, it's challenging to assess the overall impact of these changes.

  3. Consider refactoring the code to adhere to consistent coding conventions for better readability. For example, use meaningful parameter names instead of single-letter variable names like type, key, etc., to make the code more understandable.

  4. Ensure that error handling is appropriate in the calling function. Since the call to WriteSAddToBinlog has been removed, it's essential to handle errors or perform any alternative actions if necessary.

  5. Review the surrounding code and unit tests to verify that these changes do not introduce any unwanted side effects or break existing functionality.

  6. It's generally a good practice to include comments explaining the purpose and intention behind each code change to provide clear documentation for future developers.

Remember to thoroughly test the modified code to ensure its correctness and robustness after making any changes.

Expand Down