Fix replication log usage in HA
Summary: Instead of calling the wanted method, this diff takes copies of both `is_replicated(x)` and `is_active(x)` results so that they can't change during the method execution. The problem with the previous implementation was that the information about a log could change during two consecutive queries as ilustrated in the example below. ``` Thread 1 Thread2 rlog->set_active(1); if (rlog->is_replicated(1)) return true; rlog->set_replicated(1); if (rlog->is_active(1)) return false; throw InvalidLogReplicationLookup(); ``` The snippet above would throw as we don't have any information about the log `1`, because the `set_replicated` call would "shadow" the active bit. Reviewers: ipaljak Reviewed By: ipaljak Subscribers: pullbot, mferencevic Differential Revision: https://phabricator.memgraph.io/D2165
This commit is contained in:
parent
83a99aaf7d
commit
542d65544b
@ -481,16 +481,24 @@ bool RaftServer::SafeToCommit(const tx::TransactionId &tx_id) {
|
||||
// circuit the check to always return true if in follower mode.
|
||||
return true;
|
||||
case Mode::LEADER:
|
||||
// If we are shutting down, but we know that the Raft Log replicated
|
||||
// successfully, we return true. This will eventually commit since we
|
||||
// replicate NoOp on leader election.
|
||||
if (rlog_->is_replicated(tx_id)) return true;
|
||||
// We are taking copies of the rlog_ status here so we avoid the case
|
||||
// where the call to `set_replicated` shadows the `active` bit that is
|
||||
// checked after the `replicated` bit. It is possible that both `active`
|
||||
// and `replicated` are `true` but since we check `replicated` first this
|
||||
// shouldn't be a problem.
|
||||
bool active = rlog_->is_active(tx_id);
|
||||
bool replicated = rlog_->is_replicated(tx_id);
|
||||
|
||||
// Only if the transaction isn't replicated, thrown an exception to inform
|
||||
// If we are shutting down, but we know that the Raft Log replicated
|
||||
// successfully, we return true. This will eventually commit since we
|
||||
// replicate NoOp on leader election.
|
||||
if (replicated) return true;
|
||||
|
||||
// Only if the transaction isn't replicated, thrown an exception to inform
|
||||
// the client.
|
||||
if (exiting_) throw RaftShutdownException();
|
||||
if (exiting_) throw RaftShutdownException();
|
||||
|
||||
if (rlog_->is_active(tx_id)) {
|
||||
if (active) {
|
||||
if (replication_timeout_.CheckTimeout(tx_id)) {
|
||||
throw ReplicationTimeoutException();
|
||||
}
|
||||
|
@ -62,10 +62,7 @@ class ReplicationLog final {
|
||||
}
|
||||
}
|
||||
|
||||
bool is_active() const {
|
||||
if (flags_ & REPLICATED) return false;
|
||||
return flags_ & ACTIVE;
|
||||
}
|
||||
bool is_active() const { return flags_ & ACTIVE; }
|
||||
|
||||
bool is_replicated() const { return flags_ & REPLICATED; }
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user