Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15828 )
Change subject: [master-test] added GetTableSchema() microbenchmarks ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/15828/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15828/1//COMMIT_MSG@13 PS1, Line 13: debug > yea I think quoting release build numbers is probably more interesting Added stats for RELEASE build as well. Interestingly enough, in the case of RELEASE build the difference is huge for function call counters. http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1084 PS1, Line 1084: FLAGS_rpc_service_queue_length > Probably, this should be min(FLAGS_rpc_service_queue_length, the number of From the other side, it's desirable to test with as many threads as possible. The only requirement is to avoid RPC queue overflows. http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1110 PS1, Line 1110: if (resp.has_error()) { : LOG(WARNING) << SecureDebugString(resp); : continue; : } > Do you think it's worth failing this test on error? That way we can more ea Indeed, that's a better approach. Done. http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1137 PS1, Line 1137: 200 > Probably, this should depend on the number of CPU cores. >From the other side, it's desirable to test with many threads each >representing a client calling GetTableSchema(). http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1139 PS1, Line 1139: const bool supports_authz = GetParam(); : FLAGS_master_support_authz_tokens = supports_authz; : : for (auto idx = 0; idx < kNumTables; ++idx) { : auto table_name = Substitute("testtb_$0", idx); : EXPECT_OK(CreateTable(table_name, kTableSchema)); : } > nit: consider putting these in some common SetUp()? Moved setting supports_authz_ into the constructor. In different scenarios kNumTables is different, so SetUp() would not fit for the piece of the creation of tables, IMHO. http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1180 PS1, Line 1180: auto stopper_cleanup = MakeScopedCleanup([&]() { : for (auto& t : caller_threads) { : t.join(); : } : }); > nit: just FYI, you can use this too if you'd like, especially if you don't Thanks! I always forget about those useful macros. http://gerrit.cloudera.org:8080/#/c/15828/1/src/kudu/master/master-test.cc@1191 PS1, Line 1191: calls > nit: maybe "function calls"? an RPC is also a "call", so it wasn't quite cl Done -- To view, visit http://gerrit.cloudera.org:8080/15828 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1f3d4fe1df426d50bd465c8ca7bbfb6924bffe9 Gerrit-Change-Number: 15828 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 28 Apr 2020 08:21:05 +0000 Gerrit-HasComments: Yes
