Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19584 )
Change subject: [KUDU-3452] A tool to report on table creation progress ...................................................................... Patch Set 11: (2 comments) It would be good if we can get the 'state' of all tables from kudu master and filter out 'CREATING' tables based on this attribute on the client side. However, the state of a table can be 'RUNNING' even if it has some 'CREATING' tablets. This patch filter out tables with 'CREATING' tablets on the server side. Considering it's different from the original 'ListTables' method, using a separate method/rpc would be better than adding a new branch to the existing method, what do you think? http://gerrit.cloudera.org:8080/#/c/19584/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19584/11/src/kudu/master/catalog_manager.cc@3951 PS11, Line 3951: if (req->has_show_in_flight_tables() && req->show_in_flight_tables()) { : for (const auto& table_entry : table_ids_map_) { : scoped_refptr<TableInfo> table = table_entry.second; : TableMetadataLock table_lock(table.get(), LockMode::READ); : if (table_lock.data().is_deleted()) { : continue; : } : vector<scoped_refptr<TabletInfo>> tablets; : table->GetAllTablets(&tablets); : int num_tablets_in_flight = 0; : for (const auto& tablet : tablets) { : TabletMetadataLock tablet_lock(tablet.get(), LockMode::READ); : if (!tablet_lock.data().is_creating()) { : continue; : } : num_tablets_in_flight++; : } : // Creating table has 2 steps. The first step is to create the metadata : // of the table in catalog manager and after that the status of the table : // will be 'RUNNING'. The second step is to create all replicas of the table : // asynchronously. : // The table is on creating or altering when the number of creating tablets : // is not 0. : if (num_tablets_in_flight <= 0) { : continue; : } : : ListTablesResponsePB::TableInfo* table_info = resp->add_tables(); : table_info->set_id(table->id()); : table_info->set_name(table_lock.data().name()); : table_info->set_num_tablets(tablets.size()); : table_info->set_num_tablets_in_flight(num_tablets_in_flight); : table_info->set_state(table_lock.data().pb.state()); : } : return Status::OK(); : } : This code block seems used to implement a standalone function. It would be better if it could be a separate method, instead of being embedded in the other method . Also, I think it can even be wrapped into a separate rpc method. http://gerrit.cloudera.org:8080/#/c/19584/11/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/19584/11/src/kudu/tools/tool_action_table.cc@890 PS11, Line 890: FLAGS_show_in_flight_tables = true; If this flag cannot be set by users, could we just use it as a parameter of 'TableLister::ListTablets'? -- To view, visit http://gerrit.cloudera.org:8080/19584 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I348b69f48e6ce36ed869097f9f798c5946136de5 Gerrit-Change-Number: 19584 Gerrit-PatchSet: 11 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 24 May 2023 11:35:58 +0000 Gerrit-HasComments: Yes
