David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/5345/1/src/kudu/integration-tests/delete_table-test.cc File src/kudu/integration-tests/delete_table-test.cc: PS1, Line 1280: 65536 : 1024; this is weird. - Magic numbers? use constants? - Is this a count or the number of rows? I thought it was a count at first but then I saw that you use this in SetBatchSizeBytes PS1, Line 1285: w.set_table_name("table-to-delete"); if you don't care about this don't set it PS1, Line 1286: // Actually, need just one tablet to make sure the test exercises : // the scenario of deleting a tablet when fetching data from it, : // not when opening the next tablet to fetch data from. However, 2 is the : // minimum possible for TestWorkload. Setting this to 2 gives the desired : // result anyway: the table is split-partitioned evenly across the key space : // and the test inserts just a few thousand rows -- all rows are : // in the first partition. if you don't call set_num_tablets the default is 1 (not sure why you wouldn't be allowed to set though) PS1, Line 1294: w.set_write_pattern(TestWorkload::INSERT_SEQUENTIAL_ROWS); do you care that the rows are sequential? random would be best. also it would be good to make sure that there are flushes/compactions going on (i.e if you insert all these rows into the memrowset who's to say that we actually hang on to the blocks of diskrowsets?) PS1, Line 1300: 8 hum that's a funny number. why 8? (not saying its "wrong" just curious why you picked this one) PS1, Line 1311: rows_to_insert / 4 i don't understand this. PS1, Line 1318: Now, remove "Now" (I've done this a lot in the past and have been told to remove by the native speakers :)) PS1, Line 1322: advertized typo PS1, Line 1324: vector<string> tablets; : do { : SleepFor(MonoDelta::FromMilliseconds(256)); : tablets = inspect_->ListTablets(); : } w what dinesh said about the utils PS1, Line 1341: scanner.Close(); is there a specific need to close? it'll be closed() on scope exit iirc -- To view, visit http://gerrit.cloudera.org:8080/5345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
