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 21: (18 comments) http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/client/client.h@1314 PS21, Line 1314: /// @note This statistic is pre-replication. Given the feature is currently experimental, can you also add a note? Especially if the idea is to expand this feature with database support, the public API may change. Adding such a note will inform users to watch out for API changes. Same below, and with the KuduTableAlterer. http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/client/client.h@1328 PS21, Line 1328: std::string ToString(bool show_limit = false) const; I don't think adding arguments (even arguments with defaults) is ABI compatible -- programs compiled with the client library without this argument using a newer version may break. >From >https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B: "You cannot... change its signature. This includes:... extending a function with another parameter, even if this parameter has a default argument... If you need to add extend/modify the parameter list of an existing function, you need to add a new function instead with the new parameters." I'd suggest either adding a separate function ToString(bool show_limit) that calls ToString() if 'show_limit' is false, or just removing the argument and printing the limits regardless. http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc File src/kudu/integration-tests/write_quota-itest.cc: http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@128 PS21, Line 128: ScanWholeTables nit: "Table" should be singular http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@154 PS21, Line 154: row_count_limit_for_disk_size_test nit: use kCamelCase? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@197 PS21, Line 197: Status DeleteTable() { : return client_->DeleteTable(kTableName); : } : Unused? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@216 PS21, Line 216: FLAGS_heartbeat_interval_ms Would it speed this test up to set this to a low value? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@257 PS21, Line 257: k--; k >= 0; k-- nit: in my opinion, it'd be easier to read this with the more traditional for (k = 0; k < row_count_limit_for_disk_size_test; i++) http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@270 PS21, Line 270: 1000 Rather than waiting and hoping for certain invariants to become true, it may be useful to use ASSERT_EVENTUALLY in some places. http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@351 PS21, Line 351: LOG(INFO) << "Refuse the table limit change if the user is not superuser..."; nit: many of these INFO logs seem more useful as a code comments than glog messages http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@402 PS21, Line 402: on_disk_size_limit nit: use kCamelCase for const variables, per the Google style guide. Same elsewhere http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/integration-tests/write_quota-itest.cc@407 PS21, Line 407: NO_FATALS(TestRowLimit()); Should we also tet the size limit? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@2754 PS21, Line 2754: if (req.has_disk_size_limit() || req.has_row_count_limit()) { : RETURN_NOT_OK(AlterTableLimitRpcLeaderLocked(req, resp, user)); : } Did you consider baking this into AlterTable() directly? As it stands, we'll take the table lock twice and write to the catalog twice, which isn't great. At first glance it might not take too much work to reuse AlterTable(), but I'm not sure. http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3417 PS21, Line 3417: resp->set_disk_size_limit(TableInfo::TABLE_WRITE_DEFAULT_LIMIT); : resp->set_row_count_limit(TableInfo::TABLE_WRITE_DEFAULT_LIMIT); nit: why not just _not_ set the field, and rely on callers to use has_disk_size_limit() and has_row_count_limit()? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3424 PS21, Line 3424: scoped_refptr<TableInfo> table = : FindPtrOrNull(normalized_table_names_map_, NormalizeTableName(table_name)); Rather than doing this lookup again, can we pass the 'table' instance from L3249? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3426 PS21, Line 3426: if (PREDICT_TRUE(table)) { nit: it helps a little with readability to write this as: if (!table) { return true; } uint64_t disk_size = 0; ... return disallow_write; http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3435 PS21, Line 3435: TableMetadataLock table_lock(table.get(), LockMode::READ); By the time this is called, isn't the lock already taken at L3257? Maybe we can instead pass a const ref of the table metadata PB to this function instead. http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3443 PS21, Line 3443: table_disk_size_limit != TableInfo::TABLE_WRITE_DEFAULT_LIMIT I've asked this elsewhere, but could we instead use table_lock.data().pb.has_table_disk_size_limit() instead of using a magic number? http://gerrit.cloudera.org:8080/#/c/17273/21/src/kudu/master/catalog_manager.cc@3462 PS21, Line 3462: disallow_write ? "forbidden" : "allowed") nit: this will always be "forbidden" -- 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: 21 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 20 Apr 2021 04:11:32 +0000 Gerrit-HasComments: Yes
