fix: migration finalization process #4576
Conversation
BorysTheDev
changed the title
BorysTheDev
force-pushed
the
fix_migration_finalization_process
branch
from
479031d to
dc22dc9
Compare
BorysTheDev
changed the title
| [&](const auto& flow) { return flow->GetLastAttempt() == attempt; }))) { | ||
| // if data was sent after LSN, WaitFor() always returns false so to reduce wait time | ||
| // we check current state and if WaitFor false but GetLastAttempt() == attempt | ||
| // the Join is failed and we can return false |
There was a problem hiding this comment.
Is this really true? I enter the body of the while(true) for the first time and I call
wait_res = bc_->WaitFor(wait_time) and it returns false because it got notified. Now I check with std::_all_of and it returns true. Why did the migration fail here ? Isn't that the happy path ?
There was a problem hiding this comment.
No because waitFor returns false if data was sent after LSN or if attempt isn't correct
| if ((bc_->WaitFor(absl::ToInt64Milliseconds(timeout - passed) * 1ms)) && | ||
| (std::all_of(shard_flows_.begin(), shard_flows_.end(), | ||
| [&](const auto& flow) { return flow->GetLastAttempt() == attempt; }))) { | ||
| // if data was sent after LSN, WaitFor() always returns false so to reduce wait time |
There was a problem hiding this comment.
Maybe instead of
bc_->Add(); // the flow isn't finished so we lock it again
in line 95 you should do bc_->Cancle() ? wouldnt this fix the issue?
There was a problem hiding this comment.
In this case, we need to change our migration finalize algorithm. The current fix should work OK. let's merge it and then we can discuss changes, because we want to add the force finish if Pause failed
BorysTheDev
force-pushed
the
fix_migration_finalization_process
branch
from
f08c722 to
872e6cb
Compare
BorysTheDev
force-pushed
the
fix_migration_finalization_process
branch
from
872e6cb to
b96665e
Compare
| state_.store(MigrationState::C_FINISHED); | ||
| keys_number_ = cluster::GetKeyCount(slots_); | ||
| } | ||
| return wait_res; |
There was a problem hiding this comment.
I will say this flow is a bit confusing
In this flow we might return false here but not print warning and call ReportError as we have in the condition (passed >= timeout)
Does this makes sense?
There was a problem hiding this comment.
I see your point, to my mind it's not really needed because we send the error to the source node in anycase. But it can help a little to debug so I will add them
fixes: #4551
problem: Pause() failed and data was sent to the target node after LSN, so migration can't be finished