Yingchun Lai 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 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/19584/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19584/2//COMMIT_MSG@7 PS2, Line 7: tables Since there two cases, creating tables and adding range partions, so 'tablets' may be more suitable? http://gerrit.cloudera.org:8080/#/c/19584/2//COMMIT_MSG@18 PS2, Line 18: kudu table list nit: 'kudu table list' http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/client/client-internal.cc@483 PS2, Line 483: list_creating_tables nit: list_creating_tablets? http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/client/client-internal.cc@519 PS2, Line 519: if (list_creating_tables) { CLI tool is allowed to access servers in old version, it's needed to deal the case that the server doesn't support 'list_creating_tables'. http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/client/client-internal.cc@520 PS2, Line 520: info.id = table.id(); IMO table id is an internal concept, but not user faced, why expose it to end users? http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/master/catalog_manager.cc@3949 PS2, Line 3949: return Status::OK(); Why return here, the infos bellow e.g. soft deleted tables, can be skipped to collect? http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/master/master.proto@634 PS2, Line 634: optional bool list_creating_tables = 5; nit: add '[default = false]' to explictly declare it, deal the case of using old CLI tool to operate on newer version server. Maybe 'show_soft_deleted' need add 'default = false' too? @kedeng http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/19584/2/src/kudu/tools/tool_action_table.cc@281 PS2, Line 281: continue; It seems --list_creating_tables is not work with some other flags, use GROUP_FLAG_VALIDATOR to check this. -- 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: 2 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: Tue, 14 Mar 2023 03:33:08 +0000 Gerrit-HasComments: Yes
