Todd Lipcon has posted comments on this change. Change subject: tablet copy: Make the StartTabletCopy() RPC async ......................................................................
Patch Set 2: (7 comments) > I think we need to run the whole process on the thread pool because if there > is a long delay between opening the tablet bootstrap session and finishing > it, the session could time out on the remote and the process would fail. If we used an entirely separate pool, we could set its queue length to 0 such that submitting the task would fail if there are no free slots, and thus avoid the issue. Or, use the same pool but check the queue length before submission (racy but good enough?) That said, I suppose that having the queue might be nice so that we can see that the requests are pending. Relevant to that, can you set the status listener message to indicate that it's waiting on the queue during this time, so that it's easier to know what's going on with the tablet? http://gerrit.cloudera.org:8080/#/c/5045/2/src/kudu/tserver/tablet_copy_client_session.h File src/kudu/tserver/tablet_copy_client_session.h: Line 37: class TabletCopyClientSession { doc. Line 40: ThreadPool* pool_, nit: no '_' here Line 43: std::function<void()> success_callback, for the callbacks, please specify which threadpool they'll be run on. PS2, Line 43: std::function<void()> success_callback, : std::function<void(const std::string& msg, const Status& s)> failure_callback); agree with Dinesh that a single status callback seems easier to manage. What's the purpose of the separate 'msg' aside from the status's message? Line 48: Status StartAsync(); doc when this would return a bad status PS2, Line 48: Status StartAsync(); : void DoCopy(); docs. Should this be private? http://gerrit.cloudera.org:8080/#/c/5045/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 438: this->LogAndTombstone( the old comment on line 450-452 seems to be not respected here -- if we failed to open but successfully copied, we don't need to tombstone it, right? -- To view, visit http://gerrit.cloudera.org:8080/5045 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95c63f2bfd67624844447862efbdba9cb3676112 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
