Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9266 )
Change subject: KUDU-2274. RaftConsensus should not access cmeta when shutdown ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/integration-tests/ts_tablet_manager-itest.cc@412 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 her Done http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/master/catalog_manager.cc@4365 PS2, Line 4365: catalog_status_ = s.CloneAndPrepend("ConsensusState is not available"); > I think it's worth explicitly setting the status to ServiceUnavailable() he Added a DCHECK. http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_copy_source_session.cc@194 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 t There isn't anything preventing shutting down consensus while this is running so I think we want to leave this here. http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_copy_source_session.cc@194 PS2, Line 194: RETURN_NOT_OK_PREPEND(consensus->ConsensusState(&cstate), : "Consensus state not available"); > Would I can do that if we want to guarantee that there will be no side effects to the out-param on failure. Actually the implementation already does that so I can do this and make that guarantee in the header file docs. http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_server-test.cc@423 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 e That is just how failure works (you need both calls) so I think we should leave it. http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tablet_service.cc@1220 PS2, Line 1220: continue; > Does it make sense to add before continue Done http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/9266/2/src/kudu/tserver/tserver_path_handlers.cc@294 PS2, Line 294: Status s = consensus->ConsensusState(&cstate); : if (s.ok()) { : consensus_state_html = ConsensusStatePBToHtml(cstate); : } > nit: maybe just Done -- To view, visit http://gerrit.cloudera.org:8080/9266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3f3f75674d5739b281e7c689ddb2d433b9b7415 Gerrit-Change-Number: 9266 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Sat, 10 Feb 2018 22:14:22 +0000 Gerrit-HasComments: Yes
