Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )
Change subject: [tserver] add test to reproduce KUDU-1587 conditions ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4368 PS1, Line 4368: num_data_dirs Would it also make sense to reduce the number of apply threads to 1? Would that give us any stronger guarantees? Or is the idea that it's better to test with multiple cores to be more realistic? http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4387 PS1, Line 4387: re the nit: incomplete? http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4446 PS1, Line 4446: EXPECT_LT(h->MaxValue(), : kLatencyMs * 1000 * kNumCalls / base::NumCPUs()); : EXPECT_LT(h->ValueAtPercentile(99), : kLatencyMs * 1000 * kNumCalls / (2 * base::NumCPUs())); : EXPECT_LT(h->ValueAtPercentile(50), : kLatencyMs * 1000 * kNumCalls / (4 * base::NumCPUs())); Making sure I understand these assertions: if we had a single core, we'd expect the apply queue time to be far from `kLetancySec * kNumCalls` because some of the requests should have been rejected. And I suppose the exact value for the p50 and p99 calls will be determined based on the CoDel algorithm. http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4463 PS1, Line 4463: : EXPECT_LT(h->MaxValue(), kNumCalls / 2); : EXPECT_LT(h->MeanValue(), kNumCalls / 4); Same here, right? These multipliers will be adjusted depending on the implementation of CoDel, right? Or are these guarantees/measurements made by the algorithm? http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4468 PS1, Line 4468: // Some calls should be rejected by the apply queue, so the maximum queue : // time should be much lower than xxx. nit: move this above L4436? Or is this referring to the RPC queue? -- To view, visit http://gerrit.cloudera.org:8080/16312 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d Gerrit-Change-Number: 16312 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Aug 2020 21:48:19 +0000 Gerrit-HasComments: Yes
