Dinesh Bhat has posted comments on this change. Change subject: [tools] Manual recovery tools (part 1) ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/4834/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 941: const int kAddOnTsIndex = 3; > Hmm. Instead, maybe you can change the various RunAction routines to always Sounds like good idea, let me do that in a follow-up patch, updated current callsite to RunActionStdoutNone. PS1, Line 974: > OK. Why do we need to wait for 1000 rows to be inserted, though? Picked 1000 out of thin air, to have few KB worth of data in the table, and thereby some data on the replica for the copy to actually matter. Will adding a small comment help here ? http://gerrit.cloudera.org:8080/#/c/4834/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 121: } > This would be more robust if placed in ToolTest's destructor. Done PS2, Line 936: TEST_F(ToolTest, TestCopyRemoteReplicaCopy) { : const string kTestDir = GetTestPath("test"); : MonoDelta kTimeout = MonoDelta::FromSeconds(30); : const int kSrcTsIndex = 1; : const int kDstTsIndex = 2; > Should now refer to "remote_replica copy" rather than "tablet copy". Done http://gerrit.cloudera.org:8080/#/c/4834/2/src/kudu/tools/tool_action_remote_replica.cc File src/kudu/tools/tool_action_remote_replica.cc: Line 312: req.set_caller_term(std::numeric_limits<int64_t>::max()); > AFAICT, this means that if the destination tserver already has this replica That's the intent yeah, although now that I think about it, it may be a nice idea to keep this under a --force flag in case user accidentally specifies a destination server where replica is healthy. What do you suggest ? http://gerrit.cloudera.org:8080/#/c/4834/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 916: const string& tablet_id = meta->tablet_id(); > You added the new one, but you didn't chain the old variant into it. My bad, I probably didn't interpret the word 'chain' correctly earlier :), made the non-static variant to call static now. -- To view, visit http://gerrit.cloudera.org:8080/4834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I113a25e9b6c14f7c3814140917b61e35030b58d0 Gerrit-PatchSet: 3 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
