Wang Xixu 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 9: (17 comments) http://gerrit.cloudera.org:8080/#/c/19584/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19584/8//COMMIT_MSG@7 PS8, Line 7: A tool to report on table creatio > a tool to report on table creation progress Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.h@147 PS8, Line 147: pb.state() == SysTabletsEntryPB > Would it be more straightforward to rely on SysTabletsEntryPB::CREATING and Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.cc@3955 PS8, Line 3955: if (table_lock.data().is_deleted()) { : continue; : } > What about soft-deleted tables? Should they be excluded from the output as Soft-deleted tables are took as running tables, see the function is_running() in catalog_manager.h. They are exclude not here, but in line 3963. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.cc@3968 PS8, Line 3968: Creating table has 2 steps. The first step is to create the met > Sorry, I cannot parse this. Could you try to rephrase this a bit? Here, I mean creating table has two steps. The first step is to create metadata in catalog manager and the status of the table will be running. The second step is to create all the replicas of the table asynchronously. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto@633 PS8, Line 633: Set this field to 'true' to sho > Set this field to 'true' to show information on tables in the process of be Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto@634 PS8, Line 634: show_in_flight_tables = > 'show_in_progress_tables' or 'show_in_flight_tables' ? Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto@653 PS8, Line 653: num_tablets_in_fligh > Is this number of tablets already created, tablets yet to be created, or ta The number of tablets in the process of being created. I will rename it num_tablets_in_flight. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9370 PS8, Line 9370: ListInFlightTablesTest : > It's quite obvious whether it's parameterized or by the way how it's used. Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9514 PS8, Line 9514: Contain an unknown table > Found unexpected table Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9522 PS8, Line 9522: ASSERT_OK(mini_cluster_->mini_tablet_server(0)- > What this into ASSERT_OK()? Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9530 PS8, Line 9530: Wait until there aren't in-flight ta > Wait until there aren't in-flight tablets (i.e. tablets in progress of bein Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9536 PS8, Line 9536: out; > Should this be 'out' instead of 's.String()'? Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool.proto@549 PS8, Line 549: num_tablets_in_fligh > Is this number of tablets already created, tablets yet to be created, or ta The number of tablets in flight, I will rename it num_tablets_in_flight http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@238 PS8, Line 238: > ? When the flag --show_in_flight_tables is set true, flags --list_tablets, --show_tablet_partition_info show_hash_partition_info and show_table_info should not be set. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@242 PS8, Line 242: if (FLAGS_show_in_flight_tables && > There should be an error messages explaining what's wrong and how to fix th Done http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@288 PS8, Line 288: } : > What if the server doesn't know about these two new fields and hasn't popul Thanks for your reminding. To be compatible with old version server, it needs to check whether the field 'state' existed. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@346 PS8, Line 346: table_info_pb->set_name(tname); > How do you know that this information is valid? What if the server doesn't Thanks for your reminding. To be compatible with old version server, I will check whether the field 'state' empty. -- 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: 9 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: Mon, 22 May 2023 03:53:55 +0000 Gerrit-HasComments: Yes
