David Ribeiro Alves has posted comments on this change. Change subject: Add Reinserts to tablet_history_gc-itest ......................................................................
Patch Set 10: (16 comments) http://gerrit.cloudera.org:8080/#/c/4997/10//COMMIT_MSG Commit Message: Line 17: this was already flaky so we'll see how troublesome that is.. > I'm looking into this right now, trying to pick up from where Alexey left o I unflaked the test prior to this patch by reverting to manual flush http://gerrit.cloudera.org:8080/#/c/4997/10/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: Line 21: > nit: please add <unordered_set> include file Done PS10, Line 286: int32_t rows_inserted_ = 0; > Consider removing this and replacing with a local variable, like rows_delet Done PS10, Line 317: SetStringCopy > nit: SetString() now does copying; so consider replacing SetStringCopy() wi Done Line 396: // TODO: Allow for reinsert onto deleted rows after KUDU-237 has been > Remove this comment, also should we consider treating insert and reinsert a the mechanics are quite different and it would be a bit of a refactoring so I'm going to push back on this. While cleaner in a sense I don't think it would test more than we're testing now. PS10, Line 506: MaterializedTestRow test_row > Why not to use reference instead? Why is it necessary to make a copy and t we don't take non-const references as per our style. changed this to a pointer PS10, Line 507: CHECK_EQ > Why not ASSERT_EQ() ? Done PS10, Line 556: MaterializedTestTable snapshot = CloneLatestSnapshot(); > Is it crucial to make a snapshot every time regardless of number of deleted Done PS10, Line 565: break > Consider moving this up into the very beginning of the scope next to the 'i Done PS10, Line 567: reinserts > nit: consider adding call to reserve the space: regardless of the number of num_rows_to_reinsert we only do so for rows that were previously deleted so we're not really sure how big the vector needs to be. You do make a good point about the name of the var. Will change it to max_rows_to_reinsert. PS10, Line 569: int32_t > nit: consider adding const specifier for better readability: Done Line 569: int32_t row_key = *std::next(deleted_rows_.begin(), random.Uniform(num_deleted_rows)); > hrm, O(k*n) kind of sucks, i wonder if this needs to be made faster maybe it could, but this is not perf-critical code, right? so, does it matter? I doubt very much that this shows up on any perf trace we take of this test. PS10, Line 570: MaterializedTestRow test_row > Why not to use a reference instead of copy here? If using reference, then we don't take non-const references as per our style. changed this to a pointer PS10, Line 571: CHECK_EQ > Why not just ASSERT_EQ() ? Done PS10, Line 602: 20000 > nit: may be, create a constant for this timeout and use it here and in all Done PS10, Line 617: VLOG(1) << "Reinserted " << reinserts.size() << " rows"; > Is the 'reinserts' container valid at this point? There is set_reinserts(s ah good catch. fixed -- To view, visit http://gerrit.cloudera.org:8080/4997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
