Hongjiang Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17273 )
Change subject: KUDU-3223: Quota management for table write ...................................................................... Patch Set 26: (10 comments) http://gerrit.cloudera.org:8080/#/c/17273/16/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/16/src/kudu/master/catalog_manager.cc@334 PS16, Line 334: TAG_FLAG(enable_per_range_hash_schemas, unsafe); : : DEFINE_bool(enable_table_write_limit, false, : "Enable the table write limit. " : "When the table's size or row count is approaching the limit, " : "the write may be forbidden."); : TAG_FLAG(enable_table_write_limit, experimental); : : DEFINE_int64(table_disk_size_limit, : std::numeric_limits<int64_t>::max(), : "Set the target size of a table to write"); : > Thanks for doing this research! I'm on board with adding both a table level That needs some time I think. Well, I'll check this. http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2887 PS26, Line 2887: modify the table limit by admin > nit: for consistency with other comments in this method, capitalize and add Ack http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2888 PS26, Line 2888: if (req.has_disk_size_limit() || req.has_row_count_limit()) { > I wonder if it's worth enforcing that we are only able to change limits if Good. Limit change can only be done by superuser, typically, it should not combine with other alter table operations. http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2888 PS26, Line 2888: if (req.has_disk_size_limit() || req.has_row_count_limit()) { : if (user && !master_->IsServiceUserOrSuperUser(*user)) { > nit: could we put this into the authz_func above so all of our authorizatio Ack http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2894 PS26, Line 2894: if (req.has_disk_size_limit()) { : l.mutable_data()->pb.set_table_disk_size_limit(req.disk_size_limit()); : } : if (req.has_row_count_limit()) { : l.mutable_data()->pb.set_table_row_count_limit(req.row_count_limit()); : } > If we are removing the limits, can we call pb.clear_table_disk_size_limit() Yes, this can be improved. But I don't think we can save any space. Considering user wants to clear the limit, the request should have a flag to inform us, for example, "clear_disk_size_limit" to indicate this change, now the special value is the flag. http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2900 PS26, Line 2900: { > nit: don't need the extra {}s? Ack http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@2903 PS26, Line 2903: Status s = sys_catalog_->Write(std::move(actions)); : if (PREDICT_FALSE(!s.ok())) { : LOG(ERROR) << "An error occurred while updating table limit on sys-tables: " : << s.ToString(); : return s; : } > This is still being called twice, once here and once at L3085. It's quite a Yes, it should be merged http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@3433 PS26, Line 3433: d > nit: add a period Ack http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@3435 PS26, Line 3435: write > nit: "writing is forbidden" Ack http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/master.proto@210 PS26, Line 210: -1 means no restriction. > I'd still prefer that the on-disk format of "no restriction" be empty, rath Ack -- 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: 26 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: Thu, 29 Apr 2021 03:31:06 +0000 Gerrit-HasComments: Yes
