fix: slotmigrate return not correct #2741
Conversation
WalkthroughThe recent modifications focus on altering the return logic of the Changes
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}
src/pika_migrate_thread.cc
Outdated
| return 0; | ||
| } else { | ||
| LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: "<< key <<" type: " << static_cast<int>(type) << " is illegal"; | ||
| return -1; | ||
| } | ||
| if (slot_id != slot_id_) { | ||
| LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne Slot : " << slot_id << " is not the migrating slot:" << slot_id_; | ||
| return -2; | ||
| return -1; |
There was a problem hiding this comment.
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
elseblock withreturn -1;prevents theif (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
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/pika_migrate_thread.cc (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.cc
chejinge
force-pushed
the
slotmigrateone
branch
2 times, most recently
from
d3a0430
to
cc8de39
Compare
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/pika_migrate_thread.cc (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.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/pika_migrate_thread.cc (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.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/pika_migrate_thread.cc (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.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/pika_migrate_thread.cc (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.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/pika_migrate_thread.cc (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.cc
| } | ||
|
|
||
| if (slot_id != slot_id_) { |
There was a problem hiding this comment.
这个判断还需要加吗？
不对slot_id校验直接走下面逻辑塞进队列？
There was a problem hiding this comment.
这个判断还需要加吗？
不对slot_id校验直接走下面逻辑塞进队列？
嗯嗯，等于是不管这个 key 所属的 slot 是否在迁移，我都能把这个 key 迁走
There was a problem hiding this comment.
这个判断还需要加吗？
不对slot_id校验直接走下面逻辑塞进队列？
嗯嗯，等于是不管这个 key 所属的 slot 是否在迁移，我都能把这个 key 迁走
如果你不是所属迁移的slot你也不会进入到这里来吧，因为你不可能是migrating状态
src/pika_migrate_thread.cc
Outdated
| @@ -675,7 +676,7 @@ int PikaMigrateThread::ReqMigrateOne(const std::string& key, const std::shared_p | |||
| is_migrating_ = true; | |||
| usleep(100); | |||
| } | |||
| } else { | |||
| } | |||
| // check the key is migrating | |||
| std::pair<const char, std::string> kpair = std::make_pair(key_type, key); | |||
src/pika_migrate_thread.cc
Outdated
| @@ -934,7 +934,9 @@ void *PikaMigrateThread::ThreadMain() { | |||
| { | |||
| std::unique_lock lw(workers_mutex_); | |||
| while (!should_exit_ && is_task_success_ && send_num_ != response_num_) { | |||
| workers_cond_.wait(lw); | |||
| if (workers_cond_.wait_for(lw, std::chrono::minutes(3)) == std::cv_status::timeout) { | |||
There was a problem hiding this comment.
!should_exit_ && is_task_success_ 这里我觉得不能删除 毕竟这个参数是异步赋值的如果没有赋值成功那线程就不会销毁 占用，
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/pika_migrate_thread.cc (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_migrate_thread.cc
Co-authored-by: chejinge <chejinge@360.cn>
Summary by CodeRabbit