Andrew Wong 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) Sorry for the delay on review! Looking better, most of my comments are nits, and I still have one question about comparing to other systems. 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"); : > I have investigated some databases. Thanks for doing this research! I'm on board with adding both a table level quota and eventually a namespace/database level quota, similar to HBase. I think it's best to keep the API listed as experimental so it gives us a bit more room to expand the feature footprint. One thing that may be worth looking more into is whether or not these other database systems also include their WAL sizes and metadata sizes in their sizes. You've already found it difficult to work with the fact that the "size" metric includes a large initial value for the WALs -- perhaps other systems get around this by only measuring flushed data? I'm not sure, but it would be good to understand how other systems measure their quotas, and see if that is something we can improve in Kudu. 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 a period 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 the AlterTable request is _only_ changing limits (i.e. not changing the schema, ownership, etc.). I think that would simplify the authorization a bit, since then we wouldn't require that users are both SuperUsers _and_ have regular privileges in Ranger to alter the table. What do you think? Maybe return an error if we're trying to do anything in addition to changing limits? 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 authorization is centralized? 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() and pb.clear_table_row_count_limit() rather than setting a special value? That way we're not using any space by default. 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? 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 an expensive call -- would it be possible to rely on the call below? http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@3433 PS26, Line 3433: d nit: add a period http://gerrit.cloudera.org:8080/#/c/17273/26/src/kudu/master/catalog_manager.cc@3435 PS26, Line 3435: write nit: "writing is forbidden" 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, rather than a special value like this, especially given we have the ability to clear protobuf fields. I'm fine with having a special value for the AlterTable requests though. -- 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 02:13:22 +0000 Gerrit-HasComments: Yes
