Alexey Serbin has posted comments on this change. Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test ......................................................................
Patch Set 1: (10 comments) Thank you for the review. 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. That's powers of 2. Is that weird? That's the only place they are used, what is the use for another constants? The only constant which is used is later is rows_to_insert, and this is where it's defined. I used rows_to_insert / 4 to get number of bytes which would guarantee that those rows would not be fetched at once. I could be just rows_to_insert as number of bytes, since one rows takes more than one byte, right? PS1, Line 1285: w.set_table_name("table-to-delete"); > if you don't care about this don't set it Done 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 Great: I fixed the issue with set_num_tablets() and sent it for review: http://gerrit.cloudera.org:8080/5347 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 wou Nope, I don't care much, but I just wanted to make sure they are in the same tablet. Since I could not set number of tablets to 1 explicitly before, I assumed 2 is the minimum and that's why I used sequential. Now, once the issues with num_tablets is resolved, I can use random. BTW, why random is the best? Another question: how to make sure that flushes/compactions are there? PS1, Line 1300: 8 > hum that's a funny number. why 8? (not saying its "wrong" just curious why 2^3 Which one would you prefer instead? :) PS1, Line 1311: rows_to_insert / 4 > i don't understand this. The idea is to have some number in bytes which would guarantee there would be more than one batch while fetching all rows. It might be just rows_to_insert, since every row takes more than 1 bytes, right? I put 4 just in case, but I think it make sense to leave just rows_to_insert -- will update. BTW, it seems this SetBatchSizeBytes() has no effect at all -- regardless of the setting, the scanner always output batches of 100 rows. Probably, that's another TODO. Will file JIRA for that. PS1, Line 1318: Now, > remove "Now" (I've done this a lot in the past and have been told to remove Done PS1, Line 1322: advertized > typo Done PS1, Line 1324: vector<string> tablets; : do { : SleepFor(MonoDelta::FromMilliseconds(256)); : tablets = inspect_->ListTablets(); : } w > what dinesh said about the utils I could not find proper function without much of boilerplate code to write around. I don't need those tablet server identifiers, etc. Which exact function would you recommend? Or it's better to add a new one (which incorporated this piece of code)? PS1, Line 1341: scanner.Close(); > is there a specific need to close? it'll be closed() on scope exit iirc Yes, it's just to make sure we don't hold any stuff open on the server. Or it's already so by the end of the scan? -- 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
