David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1330: Add a tool to unsafely recover from loss of majority 
replicas
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6066/7/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS7, Line 1667: msg_timestamp
> Regardless, I don't think it's ever legal to create new writes at the safe 
Mike is right that getting the safe time is wrong. You likely need to make 
TimeManager::GetSerialTimestamp() callable here (either friending or making it 
public, likely friending is slightly better IMO) to be able to obtain a 
timestamp.

I've been also mentioning for a while that the timestamp as well as the OpId 
and term all need to be part of the state snapshot that is taken under the lock.
Let me be more specific: I think block starting at LOC 1579 should look like:

int64_t all_replicated_index;
int64 last_committed_index;
OpId preceding_opid;
uint64 msg_timestamp;
  {
    ReplicaState::UniqueLock lock;
    RETURN_NOT_OK(state_->LockForRead(&lock));
    current_term = state_->GetCurrentTermUnlocked();
    committed_config = state_->GetCommittedConfigUnlocked();
    if (state_->IsConfigChangePendingUnlocked()) {
      LOG_WITH_PREFIX_UNLOCKED(WARNING)
            << "Node has a pending config, so new config will be appended. "
            << "Currently pending config on the node :"
            << state_->GetPendingConfigUnlocked().DebugString();
    }
 all_replicated_index = queue_->GetAllReplicatedIndex();
 last_committed_index = queue_->GetCommittedIndex();
 preceding_opid = queue_->GetLastOpIdInLog();
 msg_timestamp = time_manager_->GetSerialTimestamp().value;
  }

This way you get a consistent view of the whole state and an index/timestamp 
misorder is not possible


-- 
To view, visit http://gerrit.cloudera.org:8080/6066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I908d8c981df74d56dbd034e72001d379fb314700
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <[email protected]>
Gerrit-Reviewer: David Ribeiro Alves <[email protected]>
Gerrit-Reviewer: Dinesh Bhat <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

Reply via email to