Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17444 )
Change subject: KUDU-3223: Mangement of per-table limit by kudu cli ...................................................................... Patch Set 5: (3 comments) Thanks for following up with this tool! http://gerrit.cloudera.org:8080/#/c/17444/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17444/5/src/kudu/master/catalog_manager.cc@2954 PS5, Line 2954: Modify nit: "Modifying", same below http://gerrit.cloudera.org:8080/#/c/17444/5/src/kudu/master/catalog_manager.cc@3472 PS5, Line 3472: else { : resp->set_disk_size_limit(TableInfo::TABLE_WRITE_DEFAULT_LIMIT); : } Why not just not set the limit fields if there's no limit? That way we probably save some bytes on the wire. http://gerrit.cloudera.org:8080/#/c/17444/5/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17444/5/src/kudu/tools/kudu-tool-test.cc@3965 PS5, Line 3965: TEST_F(ToolTest, TestChangeTableLimitNotSupported) { Should we try changing the limit when it's not supported and ensure we get an error? Otherwise this just seems to be testing that we are able to list unspecified limits, and there's no changing of the table limit being tested. Or change the name of the test to suggest that that is what we are testing (e.g. TestReportUnspecifiedTableLimits). -- 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: 5 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 17 May 2021 01:00:11 +0000 Gerrit-HasComments: Yes
