Hongjiang Zhang 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: (15 comments) 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 Ack http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2960 PS15, Line 2960: Modifying > nit: Setting Ack 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 Ack http://gerrit.cloudera.org:8080/#/c/17444/15/src/kudu/master/catalog_manager.cc@2973 PS15, Line 2973: Modifying > nit: Setting Ack 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 limi Ack 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@3975 PS15, Line 3975: 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)); : : // Create the table. : TestWorkload workload(cluster_.get()); : workload.set_table_name(kTableName); : workload.set_schema(schema); : workload.set_num_replicas(1); : workload.Setup(); > nit: To avoid extraneous test code, we don't need to define a schema since Ack 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 ta Ack 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: Ack 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? Ther Ack 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_ Ack 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 gre Ack 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() Ack 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' Ack 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'? Ack 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 Ack -- 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: Fri, 21 May 2021 02:26:09 +0000 Gerrit-HasComments: Yes
