Alexey Serbin has posted comments on this change. ( )

Change subject: KUDU-2274. RaftConsensus should not access cmeta when shutdown

Patch Set 2:

File src/kudu/integration-tests/
PS2, Line 412: if (!s.ok()) continue; // This replica is shut down or failed.
Would it make sense to check for the specific type of the non-OK status here, 
and continue only in case of IsServiceUnavailable()?
File src/kudu/master/
PS2, Line 4365: catalog_status_ = s.CloneAndPrepend("ConsensusState is not 
I think it's worth explicitly setting the status to ServiceUnavailable() here: 
there is some logic that assumes that other components would retry in that case.

Or just add DCHECK(s.IsServiceUnavailable()) here.  I'm just concerned about 
the behavior of the clients trying to connect to the master when if we return 
anything but ServiceUnavailable here.
File src/kudu/tserver/
PS2, Line 194:   RETURN_NOT_OK_PREPEND(consensus->ConsensusState(&cstate),
Maybe, it's worth adding DCHECK_OK() here as well?  Isn't a logical error to 
initiate tablet copy source session from a tablet which has been shutdown?  Or 
that's acceptable during initialization of a tablet server?
PS2, Line 194:   RETURN_NOT_OK_PREPEND(consensus->ConsensusState(&cstate),
             :                         "Consensus state not available");

                        "Consensus state not available");

be acceptable here?
File src/kudu/tserver/
PS2, Line 423:   replica->SetError(Status::IOError("This error will leave the 
replica FAILED state at shutdown"));
             :   replica->Shutdown();
nit: maybe, separate these two cases to make sure that having just one is 
enough to get ServiceUnavailable error on while trying to call 
RaftConsensus::ConsensusState() method.
File src/kudu/tserver/
PS2, Line 1220:       continue;
Does it make sense to add before continue


File src/kudu/tserver/
PS2, Line 294:         Status s = consensus->ConsensusState(&cstate);
             :         if (s.ok()) {
             :           consensus_state_html = ConsensusStatePBToHtml(cstate);
             :         }
nit: maybe just

if (consensus->ConsensusState(&cstate).ok()) {
  consensus_state_html = ConsensusStatePBToHtml(cstate);

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3f3f75674d5739b281e7c689ddb2d433b9b7415
Gerrit-Change-Number: 9266
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Comment-Date: Sat, 10 Feb 2018 02:22:52 +0000
Gerrit-HasComments: Yes

Reply via email to