Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17444 )
Change subject: KUDU-3223: management of per-table limit by kudu CLI ...................................................................... Patch Set 15: Code-Review+1 (14 comments) Overall looks good! Just a request to add a few more test cases and do minor changes in the names of CLI sub-commands. http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2899 PS15, Line 2899: Alter nit: altering http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960 PS15, Line 2960: Modifying nit: Setting http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960 PS15, Line 2960: LOG(INFO) nit: if logging about setting a particular limit for a table disk size, why not to log about resetting the limit to the default setting as well? http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973 PS15, Line 2973: Modifying nit: Setting http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973 PS15, Line 2973: LOG(INFO) nit: if logging about setting a particular limit for a table row count limit, why not to log about resetting the limit to the default setting as well? http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4043 PS15, Line 4043: KuduSchemaBuilder schema_builder; : schema_builder.AddColumn("key") : ->Type(client::KuduColumnSchema::INT32) : ->NotNull() : ->PrimaryKey(); : schema_builder.AddColumn("value") : ->Type(client::KuduColumnSchema::INT32) : ->NotNull(); : KuduSchema schema; : ASSERT_OK(schema_builder.Build(&schema)); nit: same as above, it's not necessary to define the schema for the test table since TestWorkload has one by default. http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/kudu-tool-test.cc@4086 PS15, Line 4086: master_addr, kTableName), Maybe, add the following test cases as well: * a negative value for the limit * the value greater than INT64_MAX http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@420 PS15, Line 420: disk_size_limit_str == "unlimited" nit here and below: maybe, compare this in a case-insensitive manner? There is iequals() function for that (see src/kudu/utils/string_case.h). http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@422 PS15, Line 422: safe_strtou64 Here and below: why not to use safe_strto64() to read in a number as int64_t? http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@426 PS15, Line 426: disk_size_limit = u_disk_size_limit Here and below: disk_size_limit might end up being negative if a number greater than INT64_MAX is specified. Maybe, read in the input above using safe_strto64() instead? http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@440 PS15, Line 440: row_count_limit_str == "unlimited" nit: maybe, compare this in a case-insensitive manner? There is iequals() function for that (see src/kudu/utils/string_case.h). http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1220 PS15, Line 1220: disk_size_limit Why not just 'disk_size' ? The upper context is already called 'set_limit', so duplicating 'limit' isn't necessary. http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1227 PS15, Line 1227: row_count_limit Ditto: leave just 'row_count'? http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/tools/tool_action_table.cc@1234 PS15, Line 1234: of the table nit: for a table -- To view, visit http://gerrit.cloudera.org:8080/17444 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59bb64cc85d5fca48ae8ec980f5cfc62e4b3a1d3 Gerrit-Change-Number: 17444 Gerrit-PatchSet: 15 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 20 May 2021 23:37:18 +0000 Gerrit-HasComments: Yes
