Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9393 )
Change subject: KUDU-2290: Tool to re-create a tablet ...................................................................... Patch Set 5: (13 comments) After discussion, I decided to take out the "quarantine" part of this patch: 1. It's a significant question how it would work with authz. 2. The quarantine table is by its nature not writable, and it's a question of if it should be. 3. There's overall a lot of things to consider about how such a setup would work. So now this patch will just seek to add a way to replace a tablet with a new one, deleting the data from the old one. http://gerrit.cloudera.org:8080/#/c/9393/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9393/5//COMMIT_MSG@11 PS5, Line 11: is > Nit: omit this Done http://gerrit.cloudera.org:8080/#/c/9393/5//COMMIT_MSG@19 PS5, Line 19: <table id> > Given that tablet_id is globally unique and table_name already identifies t n/a since the "quarantine" part is being removed for now. http://gerrit.cloudera.org:8080/#/c/9393/5//COMMIT_MSG@24 PS5, Line 24: its > Nit: it's n/a http://gerrit.cloudera.org:8080/#/c/9393/5//COMMIT_MSG@26 PS5, Line 26: eventually this will stop > There may be some interesting fallout from this. In particular, I believe t n/a because the "quarantine" piece is being removed. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/client/meta_cache.cc@912 PS5, Line 912: // Partition should not have changed. > I've developed a bad habit of using "partition" and "tablet" interchangeabl n/a since the "swapping" of the old tablet to a quarantine table is being removed. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/client/meta_cache.cc@919 PS5, Line 919: // In this case, 'tablets_by_key' will be empty but a RemoteTablet : // will exist in the cache. > I don't understand this case. AFAICT tablet replacement is "atomic", which n/a since the "swapping" of the old tablet to a quarantine table is being removed. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/catalog_manager.h@581 PS5, Line 581: Status ReplaceTablet(const std::string& tablet_id, master::ReplaceTabletResponsePB* resp); > It'd be nice to have testing of this new method outside of the CLI tests. B I added coverage in master-stress-test. I also added an integration test to test writing to a table as once of its tablets is replaced. This fails due to an existing bug in the client. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/catalog_manager.cc@4302 PS5, Line 4302: // To be safe, we'll take the global catalog manager lock for the rest of this method. > While I appreciate the motivation to block out everything else, I don't thi Done http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/catalog_manager.cc@4303 PS5, Line 4303: lock > Style nit: Use l (or l_foo) for lock guards. It especially helps in this ca Done http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/master.proto@707 PS5, Line 707: recovered > Nit: "manually recovered", to make it clear that any actual recovery is out n/a since the "swapping" of the old tablet to a quarantine table is being removed. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/master/master.proto@715 PS5, Line 715: > Just to close this out, Dan (and Todd) reminded me that for scalars, option Thanks Adar. http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/tools/tool_action_common.cc@593 PS5, Line 593: > Nit: these were all grouped together, so either omit this empty line, or ad Done http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/9393/5/src/kudu/tools/tool_action_tablet.cc@537 PS5, Line 537: proxy.SyncRpc<ReplaceTabletRequestPB, ReplaceTabletResponsePB>( > This returns a Status that should be checked. Done -- To view, visit http://gerrit.cloudera.org:8080/9393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbefbde68e3ca724f04efe0426a3906e5c33ed3c Gerrit-Change-Number: 9393 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 29 Mar 2018 08:35:40 +0000 Gerrit-HasComments: Yes
