Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17273 )
Change subject: KUDU-3223: Management of per-table level limit ...................................................................... Patch Set 36: (7 comments) http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/client/client.h@1809 PS36, Line 1809: , but : /// it should also support database level size limit. nit: I think this would be a bit confusing to users -- "should" is a bit ambiguous. Let's just remove this piece until database-level limits are supported. Same below. http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/client/client.h@1815 PS36, Line 1815: KuduTableAlterer* SetTableDiskSizeLimit(int64_t disk_size_limit); We should also mention that a KuduTableAlterer that is setting limits cannot perform any other operation. http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/client/table_alterer-internal.h@20 PS36, Line 20: #include <cstdint> : : #include <map> nit: please remove the extra line here. http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/integration-tests/write_limit-itest.cc File src/kudu/integration-tests/write_limit-itest.cc: http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/integration-tests/write_limit-itest.cc@352 PS36, Line 352: TestOnlySuperUserAllowedToChangeTableLimit Can you also add a test that performing any other alter operation while changing the limits will fail? http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/master/catalog_manager.h@274 PS36, Line 274: // Tables which save streaming data should has a write limit, : // for example, Kudu as a flink sinker is such an example. : // To avoid breaking the existing tables, there is no limit enforced. nit: Rather than being specific about Flink, let's keep this a bit more general, something like: "It is useful to place size limits on tables that may be ingested to by long-lived applications. To avoid breaking existing tables, there is no limit enforced." http://gerrit.cloudera.org:8080/#/c/17273/35/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/35/src/kudu/master/catalog_manager.cc@3446 PS35, Line 3446: } > But I also need table->GetMetrics()->on_disk_size, then I have to pass seve Ack http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/36/src/kudu/master/catalog_manager.cc@3461 PS36, Line 3461: table_disk_size_limit != TableInfo::TABLE_WRITE_DEFAULT_LIMIT This will never be true since table_disk_size_limit() and table_row_count_limit() return 0 if not set at L3453 and L3454. Let's write this as something like: const auto& pb = table_lock.data().pb; if (pb.has_table_disk_size_limit()) { table_disk_size_limit = pb.table_disk_size_limit(); if (table_disk_size >= table_disk_size_limit * FLAGS_table_write_limit_ratio) { disallow_write = true; } } if (!disallow_write && pb.has_table_row_count_limit... ... -- To view, visit http://gerrit.cloudera.org:8080/17273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dbf365ad59f17c0a4e2e7ea6a5afaa7680724b0 Gerrit-Change-Number: 17273 Gerrit-PatchSet: 36 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 05 May 2021 01:09:56 +0000 Gerrit-HasComments: Yes
