Alexey Serbin has posted comments on this change. Change subject: tablet copy: Allow voting from failed initial tablet copies ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/7904/2//COMMIT_MSG Commit Message: PS2, Line 7: A nit: remove the capitalization of the 'a' letter PS2, Line 13: If nit: if PS2, Line 20: OpId nit: opid I mean, consider using the same style for that: either 'opid' or 'OpId' http://gerrit.cloudera.org:8080/#/c/7904/2/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: PS2, Line 1142: int new_replica_idx = cluster_->tablet_server_index_by_uuid(new_replica_uuid); just to clarify: it's guaranteed that the new tablet replica will land at the newly added server, right? PS2, Line 1148: }); How do we know that the tablet will not start running if the ASSERT_EVENTUALLY above has been satisfied with TABLET_DATA_COPYING state? PS2, Line 1156: cluster_->tablet_server(i)->mutable_flags()->pop_back(); paranoid nit: since the arrangement of the flags passed to StartCluster() in the mutable_flags() of the tablet server is an implementation detail, would it make sense to add an assert on the last element to be tablet_copy_early_session_timeout_prob indeed? http://gerrit.cloudera.org:8080/#/c/7904/2/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS2, Line 316: MakeOpId(1, 0); I noticed there is MinimumOpId() method in opid_util.h. Why not to use that instead of MakeOpId(1, 0)? -- To view, visit http://gerrit.cloudera.org:8080/7904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibaad2f56797db731a0f701bbd37e3e450bc17b51 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
