David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9136 )
Change subject: KUDU-1613: evict replicas with wrong server UUIDs ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@9 PS4, Line 9: rebuilt what does "rebuilt" mean? http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@15 PS4, Line 15: ample servers to replicated to grammar http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc File src/kudu/consensus/consensus_peers.cc: http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@320 PS4, Line 320: case TabletServerErrorPB::TABLET_FAILED: add fallthrough expected or something http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@321 PS4, Line 321: case TabletServerErrorPB::WRONG_SERVER_UUID: nit: might make sense to reverse the order of the cases (have TabletServerErrorPB::TABLET_FAILED last) and add a comment indicating we treat WRONG_SERVER_UUID as tablet failed http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/external_mini_cluster_fs_inspector.h File src/kudu/integration-tests/external_mini_cluster_fs_inspector.h: http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/external_mini_cluster_fs_inspector.h@a52 PS4, Line 52: pull this to another changerequest? http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2750 PS4, Line 2750: test that ensures when we start a new tablet server using the : // same ports, existing tablets that previously used those ports will be : // evicted and replicated elsewhere. "tablets that used those ports"? this is confusing, please rephrase. maybe: "Test that when we reset and restart a tablet server, with a new uuid but with the same host and ports, replicas that were hosted by the previous incarnation are correctly detected as failed and eventually re-replicated." http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2755 PS4, Line 2755: is_3_4_3 this is tied to a layout (3 replicas) could you just use the flag name instead? (i.e. "prepare_replacement_before_eviction") http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2802 PS4, Line 2802: bount typo http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2805 PS4, Line 2805: env_->CreateDir check status http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2806 PS4, Line 2806: env_->CreateDir check status http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2809 PS4, Line 2809: new_ts->Start(); check status, here and elsewhere http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2810 PS4, Line 2810: LOG(INFO) << Substitute("Created new tablet server: $0 ($1)", : new_ts->uuid(), ts_opts.rpc_bind_address.ToString()); did you mean to leave this. doesn't seem super informative... -- To view, visit http://gerrit.cloudera.org:8080/9136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4 Gerrit-Change-Number: 9136 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 31 Jan 2018 17:23:35 +0000 Gerrit-HasComments: Yes
