Adar Dembo has posted comments on this change. Change subject: [tools] Manual recovery tools (part 1) ......................................................................
Patch Set 5: (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: PS1, Line 974: er_->tablet_ser > Picked 1000 out of thin air, to have few KB worth of data in the table, and Well, even a tablet with no data still has metadata that needs to be copied; an empty tablet is never a "no-op" in terms of copying. Since we're never measuring the size of the data, I don't see why we'd need to insert any rows at all. That said, I see that you're passing workload.batches_completed() into the various WaitUntilAllReplicasHaveOp() calls. Why are those calls needed? Does the test fail without them? http://gerrit.cloudera.org:8080/#/c/4834/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: PS2, Line 936: } : : // Test 'kudu tablet copy' tool when the destination tablet server is online. : // 1. Test the copy tool when the destination replica is healthy : // 2. Test the copy tool when the > Done Need to update the comments I highlighted too. At least the very first line. http://gerrit.cloudera.org:8080/#/c/4834/5/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: PS5, Line 1004: string stdout; : string stderr; Can remove these. Check the rest too. PS5, Line 1052: Explicit RPC to : // delete-with-retries What does this mean? DeleteTabletWithRetries() is a function; what does "explicit RPC" mean in this context? PS5, Line 1101: replica data size to be visible. What does this mean? 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()); > That's the intent yeah, although now that I think about it, it may be a nic Yeah, I think that may be good. This would be -1 or 0 normally, and max when --force is set. Run it by Mike though. -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
