Alexey Serbin 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 15: Code-Review+1 (20 comments) Thank you for working on this tool! Almost there! Overall, this patch looks good to me, just a few nits: syntax in comments and other minor things to correct. http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@10 PS15, Line 10: If the healthy tablet servers are not enough to create multiple : replicas. The creating task will retry continuously for a long : time. If there isn't enough healthy tablet servers to create the required number of replicas, the catalog manager retries creating corresponding tablet replicas for a long time. http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@14 PS15, Line 14: Therefore it needs a tool to show the creating tables. And users : can decide to cancel the creating tasks or not. Hence there is a need to report on the table creation process, so users could be aware of its current status. If necessary, they could make an informed decision to cancel the process by dropping the table or the partition being created if it's stuck. http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@17 PS15, Line 17: list_in_flight_tables Maybe, name this new tool 'list_in_flight'? 'kudu table' already provides the 'table' context, doesn't it? http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@18 PS15, Line 18: the creating tables tables in the process of being created http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@18 PS15, Line 18: It lists the table id, table name, : the number of total tablets, the number of creating tables and the : table's state. For each in-flight table, it reports on its id, name, state, and the number of tablets being created. http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@22 PS15, Line 22: The following is an output example Below is an example of the new tool's output: http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@24 PS15, Line 24: num_creating_tablets Shouldn't this be 'num_tablets_in_flight'? http://gerrit.cloudera.org:8080/#/c/19584/15//COMMIT_MSG@27 PS15, Line 27: num_creating_tablets ditto http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.h@727 PS15, Line 727: List the tables which are on creating or altering List all the tables in the process of being created or altered http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.cc@3962 PS15, Line 3962: table nit: a table http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.cc@3963 PS15, Line 3963: catalog manager nit: the catalog manager http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.cc@3966 PS15, Line 3966: creating tablets nit: in-flight tablets http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/catalog_manager.cc@3966 PS15, Line 3966: on creating or altering nit: in the process of being created or altered http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/master/master.proto@629 PS15, Line 629: of nit: on http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9376 PS15, Line 9376: static vector<string> TableListFormat() { : return { "pretty", "json", "json_compact" }; : } Could this be replaced with something as simple as static string kTableListFormats[] = { "pretty", "json", "json_compact", }; ? http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9383 PS15, Line 9383: TEST_P(ListInFlightTablesTest, TestListInFlightTables) { Since this new test includes scenarios that times out, add SKIP_IF_SLOW_NOT_ALLOWED() in the beginning as for other long-running Kudu tests? http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9387 PS15, Line 9387: kNumReplica nit: kNumReplicas (since kNumTabletServers) http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9390 PS15, Line 9390: // Start the cluster. nit: move this comment to line 9395? http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9399 PS15, Line 9399: Get the kudu client. nit: 'Get a Kudu client' or 'Create an instance of KuduClient' http://gerrit.cloudera.org:8080/#/c/19584/15/src/kudu/tools/kudu-tool-test.cc@9420 PS15, Line 9420: // Create a table and delete it. : { : ASSERT_OK(create_table_func(kDeleteTableName, kNumReplica)); : ASSERT_OK(client->DeleteTable(kDeleteTableName)); : } How is this relevant to the coverage that this test provides? It would be great to add a comment about that. Otherwise, maybe drop this along with kDeleteTableName constant? -- 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: 15 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: Fri, 02 Jun 2023 02:26:56 +0000 Gerrit-HasComments: Yes
