Dinesh Bhat has posted comments on this change. Change subject: [consensus] KUDU-1718: Fix few bugs around replica eviction failures ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: PS2, Line 235: // Analyze the response from peer. : PeerResponseStatus ps = AnalyzePeerResponse(); : if (PREDICT_FALSE(ps.response != PeerResponseStatus::OK)) { : VLOG_WITH_PREFIX_UNLOCKED(1) << "Error response from peer: " << ps.status.ToString() : << ": " << response_.ShortDebugString(); : if (ps.response == PeerResponseStatus::ERROR_BUT_ALIVE) { : queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid()); : } : ProcessResponseError(ps.status); > can you change this for a switch case where the error cases get a fall thro Good idea, Done. http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS2, Line 2695: // Tests KUDU-1613 and KUDU-1407 fixes when followers aren't evicted. > please be a little more discriptive Done http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 2586: virtual void HandleResponse(int attempt) OVERRIDE { > warning: parameter 'attempt' is unused [misc-unused-parameters] We are better off tackling this in a different patch since this will make other interfaces inconsistent. This variable is being unused at other places too AFAICT. PS2, Line 2589: VLOG(4) << "Error deleting the tablet " << tablet_id() << ": " : << resp_.DebugString(); > should we use a higher log level here or is there enough information in the I think the warnings below carry enough info for logging purposes, I added this if we want to be more verbose with "vmodule" option. I reduced it down to 1 looking at other VLOG levels in this file. http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: Line 476: const char* TabletServerTestBase::kTableId = "TestTable"; > warning: variable 'kTableId' defined in a header file; variable definitions Done Line 477: const char* TabletServerTestBase::kTabletId = "TestTablet"; > warning: variable 'kTabletId' defined in a header file; variable definition Done Line 478: const string TabletServerTestBase::kTestDir = "TabletServerTest-fsroot"; > warning: variable 'kTestDir' defined in a header file; variable definitions Done -- To view, visit http://gerrit.cloudera.org:8080/5111 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d3f84fd297a8e4760208a213c1ee393e92499a3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[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-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
