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 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc File src/kudu/integration-tests/write_quota-itest.cc: PS7: On Slack you mentioned having some trouble testing with size. Some thoughts on how to test this (separately from testing the number of rows): - set --log_segment_size_mb=1 so that each WAL segment is by default fairly small. - set --flush_threshold_mb=1 and --flush_threshold_secs=1 so that MRSs are flushed very aggressively - set a relatively low value for --table_size_write_limit, and just keep on inserting -- eventually Kudu should reject the writes http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@140 PS7, Line 140: const char* const kBadUser = "bad-token-user"; Remove this since it's not used? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@141 PS7, Line 141: : // Helper to get the authz token for 'table_id' from the client's cache. : static bool FetchCachedAuthzToken( : KuduClient* client, const string& table_id, SignedTokenPB* token) { : return client->data_->FetchCachedAuthzToken(table_id, token); : } : : static Status RetrieveAuthzToken(KuduClient* client, const KuduTable* table) { : const MonoDelta kTimeout = MonoDelta::FromSeconds(30); : return client->data_->RetrieveAuthzToken(table, MonoTime::Now() + kTimeout); : } : // Helper to store the authz token for 'table_id' to the client's cache. : static void StoreAuthzToken(KuduClient* client, : const string& table_id, : const SignedTokenPB& token) { : client->data_->StoreAuthzToken(table_id, token); : } Remove these, since they're not used? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@199 PS7, Line 199: // Inserts the next appropriate row to the table. : Status InsertToTable(KuduTable* table) { : shared_ptr<KuduSession> session(table->client()->NewSession()); : RETURN_NOT_OK(InsertKeyToTable(table, session.get(), next_row_key_++)); : return session->Flush(); : } Remove this too? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@220 PS7, Line 220: // Inserts the next row into the table, expecting an error. Returns the : // session error, rather than the usual coarse-grained IOError. : Status InsertToTableSessionError(KuduTable* table) { : KuduClient* client = table->client(); : shared_ptr<KuduSession> session = client->NewSession(); : RETURN_NOT_OK(InsertKeyToTable(table, session.get(), next_row_key_++)); : Status s = session->Flush(); : if (!s.IsIOError()) { : return s; : } : vector<Status> errors = GetSessionErrors(session.get()); : if (errors.size() != 1) { : return Status::RuntimeError(Substitute("expected 1 error, got $0", : errors.size())); : } : return errors[0]; : } Remove this too? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@245 PS7, Line 245: FLAGS_heartbeat_interval_ms Can you add a comment mentioning why the heartbeat interval is relevant here? Also, maybe set it to a lower value in order to speed up this test? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/integration-tests/write_quota-itest.cc@312 PS7, Line 312: TestRowLimit(); nit: here and below can you wrap this void function with NO_FATALS(). That way the reported line number will be easier to spot if this fails (rather than just getting a failure in TestRowLimit()). http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@339 PS7, Line 339: "Set the max size of a table to write"); Can you also mention that these are targets rather than strict limits? Might also be worth adding a configuration for the 0.95 value. http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@338 PS7, Line 338: 1024L * 1024 * 1024 * 1024 * 1024, : "Set the max size of a table to write"); : TAG_FLAG(table_size_write_limit, advanced); : : DEFINE_int64(table_rows_write_limit, : 1024L * 1024 * 1024 * 1024, : "Set the max rows of a table to write"); While it's great these are high values, it's still possible that these defaults will break existing users. Can we set these to -1 or somesuch as a default? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@3199 PS7, Line 3199: PREDICT nit: can we predicate this on FLAGS_enable_table_write_limit being set too? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@3356 PS7, Line 3356: LOG(INFO) << Substitute("table $0 disk_size: $1 rows: $2", table_name, *disk_size, *rows); This will likely be very verbose. Perhaps switch it to a VLOG(2)? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@3363 PS7, Line 3363: scoped_refptr<TableInfo> table = : FindPtrOrNull(normalized_table_names_map_, NormalizeTableName(table_name)); Should we return early if this is null? http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/catalog_manager.cc@3372 PS7, Line 3372: LOG(INFO) << Substitute("table $0 rows are $1, size is $2, rows limit is $3, size limit is $4", : table_name, : rows, : disk_size, : table_rows_limit, : table_disk_size_limit); Can we only log this if we're disallowed? Also consider logging this as a warning, given operators may want to do something about this quickly. http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17273/7/src/kudu/master/master.proto@209 PS7, Line 209: , -1 means no restriction. Seems ok to use -1 for the in-memory values, but for serialization, why -1 and not the unset case? -- 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: 7 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: Wed, 07 Apr 2021 21:09:32 +0000 Gerrit-HasComments: Yes
