Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9393 )
Change subject: KUDU-2290: Tool to re-create a tablet ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/CMakeLists.txt@101 PS7, Line 101: ADD_KUDU_TEST(replace_tablet-itest) Could you follow the approach Todd took in commit 1c1d3ba to decide what value of PROCESSORS to assign to this new itest? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/master-stress-test.cc File src/kudu/integration-tests/master-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/master-stress-test.cc@297 PS7, Line 297: // The tablet is gone because it was already replaced or deleted. Should we still wait in this case? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/master-stress-test.cc@468 PS7, Line 468: LOG(INFO) << "Tables created: " << num_tables_created_.Load(); What's the distribution of operations before and after your change? Curious how tablet replacement has affected things. http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/replace_tablet-itest.cc File src/kudu/integration-tests/replace_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/replace_tablet-itest.cc@63 PS7, Line 63: CHECK(tablet_id); Not needed? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/integration-tests/replace_tablet-itest.cc@76 PS7, Line 76: CHECK(proxy); Not needed? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4324 PS7, Line 4324: new_tablet = scoped_refptr<TabletInfo>(new TabletInfo(table, GenerateId())); How about: new_tablet.reset(new TabletInfo(...)); http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4346 PS7, Line 4346: // Ensure we abort the mutations if persisting the changes fails. Don't the l_table and l_tablets destructors abort automatically? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4358 PS7, Line 4358: actions.tablets_to_delete.push_back(old_tablet); Hmm, this should be in tablets_to_update, no? We never actually delete entries from the catalog table. http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4376 PS7, Line 4376: table->AddRemoveTablets({new_tablet}, {old_tablet}); : l_tablets.Commit(); : l_table.Commit(); Are you sure these three should happen with the lock_ held? They acquire locks of their own; Commit() may also sleep waiting for locks, which we shouldn't do with a spinlock held. IIRC other CatalogManager operations only hold lock_ to update global maps, then do the other steps without it. http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/master/catalog_manager.cc@4385 PS7, Line 4385: background_tasks_->Wake(); This is for the newly created tablet, right? Can you update the comment just above? http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/9393/7/src/kudu/tools/tool_action_common.h@155 PS7, Line 155: LeaderMasterProxy() = default; Nit: indentation. -- 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: 7 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: Fri, 30 Mar 2018 04:37:28 +0000 Gerrit-HasComments: Yes
