Mike Percy 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 Hrm. This is easy to change but I'm not sold on it and I think this type of comment is generally a little distracting. If we want to standardize on git commit style then we should discuss it on dev@ and reach a consensus. Then, document it. It seems somewhat counterproductive to try to enforce undocumented style standards. On the other hand, maybe I missed a discussion that happened on the list while I wasn't paying attention. PS2, Line 13: If > nit: if Done PS2, Line 20: OpId > nit: opid Well, it should be OpId. Fixed 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 t Yes, because we are explicitly calling AddServer() to do that. PS2, Line 1148: }); > Ah, that's because of tablet_copy_early_session_timeout_prob=1.0: it should correct PS2, Line 1156: cluster_->tablet_server(i)->mutable_flags()->pop_back(); > paranoid nit: since the arrangement of the flags passed to StartCluster() i This isn't the first test to use this idiom. If that implementation was changed, a bunch of tests would break, including this one. 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 tha MinimumOpid() is defined as (0,0) and that will not work because we consider it the same as "empty" in the TabletMetadata code, for historical reasons. If you're OK with it, I'll file a jira to follow up with a cleanup patch that switches the naming around. -- 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-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
