Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17273 )
Change subject: KUDU-3223: Quota management for table write ...................................................................... Patch Set 32: (19 comments) http://gerrit.cloudera.org:8080/#/c/17273/32//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17273/32//COMMIT_MSG@7 PS32, Line 7: Quota management for table write In English 'quota' has semantics of 'a fixed share of something that a person or group is entitled to receive'. That's so for OS quotas, etc where quotas (i.e. shares) for resources are defined in a per-user basis. I think naming this per-table size limit 'quota' is a misnomer. How about: management of per-table size limit ? http://gerrit.cloudera.org:8080/#/c/17273/32//COMMIT_MSG@13 PS32, Line 13: on_disk_size_limit or : row_count_limit System-wide or per-table? It would be nice to clarify. http://gerrit.cloudera.org:8080/#/c/17273/32//COMMIT_MSG@17 PS32, Line 17: quota This is not a quota per se, as I mentioned above. http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/client/table_alterer-internal.h File src/kudu/client/table_alterer-internal.h: http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/client/table_alterer-internal.h@20 PS32, Line 20: <stdint.h> nit: please use C++ header <cstdint> instead http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/integration-tests/write_quota-itest.cc File src/kudu/integration-tests/write_quota-itest.cc: http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/integration-tests/write_quota-itest.cc@237 PS32, Line 237: Scans still works nit: either 'Scanning still works' or 'Scans still work'. http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.h@274 PS32, Line 274: Stream table What is 'stream table'? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.h@274 PS32, Line 274: has have http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@340 PS32, Line 340: experimental nit: could this flag tagged with the 'runtime' tag as well? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@344 PS32, Line 344: target size For clarity, it would be great to specify the size unit (bytes, KBytes, KiBytes?) in the description. http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@349 PS32, Line 349: a table What table? It would be great if you could add more details for the description of this flag. Is this for setting some sort of default limit for every new table created? The same for the FLAGS_table_disk_size_limit above. http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@2886 PS32, Line 2886: user It would be great to add a comment to explain http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@2888 PS32, Line 2888: or super user nit: or a super user http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@2908 PS32, Line 2908: by admin nit: drop this part, it's not relevant in this context http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@2914 PS32, Line 2914: req.disk_size_limit() What if req.disk_size_limit() == -2 or other negative number but -1? I didn't see any validation performed for the incoming request, so I'm curious whether it's legit to have arbitrary negative numbers for disk size limit and what semantics does it have? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@2921 PS32, Line 2921: req.row_count_limit() ditto for the row count limit: would it be OK if the value here was -2 and what would it mean in this case? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@3417 PS32, Line 3417: disk_size nit: rename to 'table_disk_size' to match 'table_disk_size_limit' below? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@3418 PS32, Line 3418: rows nit: rename to 'table_rows' to match 'table_rows_limit' below? http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@3425 PS32, Line 3425: TableMetadataLock table_lock(table.get(), LockMode::READ); It's better to release the lock when performing IO such as logging at line 3448 below. http://gerrit.cloudera.org:8080/#/c/17273/32/src/kudu/master/catalog_manager.cc@3447 PS32, Line 3447: The table write is disallowed nit: The writing into the table is disallowed. -- 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: 32 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: Fri, 30 Apr 2021 19:11:37 +0000 Gerrit-HasComments: Yes
