Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19584 )
Change subject: [KUDU-3452] A tool to show on creating tables ...................................................................... Patch Set 8: (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 show on creating tables a tool to report on table creation progress 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: !is_running() && !is_deleted(); Would it be more straightforward to rely on SysTabletsEntryPB::CREATING and SysTabletsEntryPB::PREPARING statuses here instead? 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 well? http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/catalog_manager.cc@3968 PS8, Line 3968: The state of tables is already running before creating tablets. Sorry, I cannot parse this. Could you try to rephrase this a bit? 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: Use to show on-creating tables. Set this field to 'true' to show information on tables in the process of being created. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto@634 PS8, Line 634: list_on_creating_tables 'show_in_progress_tables' or 'show_in_flight_tables' ? http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/master/master.proto@653 PS8, Line 653: num_creating_tablets Is this number of tablets already created, tablets yet to be created, or tablets in the process of being created? 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: ListCreatingTablesParamTest It's quite obvious whether it's parameterized or by the way how it's used. I'd suggest name this test ListInFlightTablesTest 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 http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9522 PS8, Line 9522: mini_cluster_->mini_tablet_server(0)->Restart() What this into ASSERT_OK()? http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9530 PS8, Line 9530: Wait no tables that are on-creating. Wait until there aren't in-flight tablets (i.e. tablets in progress of being created). http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/kudu-tool-test.cc@9536 PS8, Line 9536: s.ToString() Should this be 'out' instead of 's.String()'? 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_creating_tablets Is this number of tablets already created, tablets yet to be created, or tablets in the process of being created? 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: ther flags is useless ? http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@242 PS8, Line 242: FLAGS_show_hash_partition_info || FLAGS_show_table_info)); There should be an error messages explaining what's wrong and how to fix the issue and be actionable. In other words, the message should explain the problem, so the operator can clearly understand how to remediate the situation. http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@288 PS8, Line 288: table_info.num_creating_tablets(), : table_info.state()); What if the server doesn't know about these two new fields and hasn't populated those, even if there are some tables in the process of being created? http://gerrit.cloudera.org:8080/#/c/19584/8/src/kudu/tools/tool_action_table.cc@346 PS8, Line 346: table_info_pb->set_num_creating_tablets(tinfo.num_creating_tablets); How do you know that this information is valid? What if the server doesn't know about the new proto fields? -- 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: 8 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: Sun, 21 May 2023 05:26:43 +0000 Gerrit-HasComments: Yes
