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: slotmigrate return not correct #2741

Merged
merged 5 commits into from
Jun 18, 2024
Merged

fix: slotmigrate return not correct #2741

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a0a5bf
fix: slotmigrate return not correct
brother-jin Jun 17, 2024
6166e47
fix: slotmigrate return not correct
brother-jin Jun 17, 2024
fa5152b
Merge branch 'OpenAtomFoundation:unstable' into slotmigrateone
chejinge Jun 18, 2024
bfc8ab4
Merge branch 'OpenAtomFoundation:unstable' into slotmigrateone
chejinge Jun 18, 2024
57cea70
fix codestyle
brother-jin Jun 18, 2024
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
Next Next commit
fix: slotmigrate return not correct
  • Loading branch information
@brother-jin
brother-jin committed Jun 17, 2024
commit 3a0a5bfcaa4aab2051c55422d0397bc3e1ae7c54
5 changes: 4 additions & 1 deletion src/pika_migrate_thread.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,11 +654,14 @@ int PikaMigrateThread::ReqMigrateOne(const std::string& key, const std::shared_p
key_type = storage::DataTypeToTag(type);
if (type == storage::DataType::kNones) {
LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: " << key << " type: " << static_cast<int>(type) << " is illegal";
return 0;
} else {
LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: "<< key <<" type: " << static_cast<int>(type) << " is illegal";
return -1;
}
if (slot_id != slot_id_) {
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.

这个判断还需要加吗？

不对slot_id校验直接走下面逻辑塞进队列？

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个判断还需要加吗？

不对slot_id校验直接走下面逻辑塞进队列？

嗯嗯，等于是不管这个 key 所属的 slot 是否在迁移，我都能把这个 key 迁走

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个判断还需要加吗？

不对slot_id校验直接走下面逻辑塞进队列？

嗯嗯，等于是不管这个 key 所属的 slot 是否在迁移，我都能把这个 key 迁走

如果你不是所属迁移的slot你也不会进入到这里来吧，因为你不可能是migrating状态

LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne Slot : " << slot_id << " is not the migrating slot:" << slot_id_;
return -2;
return -1;
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The extracted code indicates that there is a logical inconsistency. Specifically, the else block following the check for type == storage::DataType::kNones contains a return statement that would prevent the subsequent if (slot_id != slot_id_) check from ever being executed. This could lead to unintended behavior.

  • Lines 657-664: The else block with return -1; prevents the if (slot_id != slot_id_) condition from being checked.

This needs to be addressed to ensure the correct sequence of operations.

Analysis chain

Updated return logic and logging for different data types in ReqMigrateOne function.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of return values and logging messages across the codebase.

# Test: Search for similar patterns in other migration-related functions.
rg --type cpp "PikaMigrateThread::ReqMigrateOne"

Length of output: 1177

Script:

#!/bin/bash
# Extract the full implementation of the ReqMigrateOne function from the source file.
ast-grep --lang cpp --pattern 'int PikaMigrateThread::ReqMigrateOne($$$) { $$$ }' src/pika_migrate_thread.cc

Length of output: 110

Script:

#!/bin/bash
# Search for the ReqMigrateOne function signature and surrounding lines to capture the full implementation.
rg --type cpp -A 50 -B 10 "int PikaMigrateThread::ReqMigrateOne" src/pika_migrate_thread.cc

Length of output: 2129

}

// if the migrate thread exit, start it
Expand Down
1 change: 0 additions & 1 deletion src/pika_slot_command.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1440,7 +1440,6 @@ void SlotsMgrtExecWrapperCmd::Do() {
int ret = g_pika_server->SlotsMigrateOne(key_, db_);
switch (ret) {
case 0:
case -2:
res_.AppendInteger(0);
res_.AppendInteger(0);
return;
Expand Down
Loading