Alexey Serbin has posted comments on this change. Change subject: Add Reinserts to tablet_history_gc-itest ......................................................................
Patch Set 10: (13 comments) 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 PS10, Line 286: int32_t rows_inserted_ = 0; Consider removing this and replacing with a local variable, like rows_deleted, rows_updated, rows_reinserted. PS10, Line 317: SetStringCopy nit: SetString() now does copying; so consider replacing SetStringCopy() with the shorter alias PS10, Line 506: MaterializedTestRow test_row Why not to use reference instead? Why is it necessary to make a copy and then re-place the element at the same key with the copy later with snapshot[row_key] = std::move(test_row) ? PS10, Line 507: CHECK_EQ Why not ASSERT_EQ() ? PS10, Line 556: MaterializedTestTable snapshot = CloneLatestSnapshot(); Is it crucial to make a snapshot every time regardless of number of deleted rows so far? If not, consider moving this after the 'if (num_deleted_rows == 0)' check PS10, Line 565: break Consider moving this up into the very beginning of the scope next to the 'if (rows_inserted_ == 0)' check. nit: consider using 'continue' here as well as in earlier checks. Or vice versa. PS10, Line 567: reinserts nit: consider adding call to reserve the space: reinserts.reserve(num_rows_to_reinsert); PS10, Line 569: int32_t nit: consider adding const specifier for better readability: const int32_t row_key = ...; PS10, Line 570: MaterializedTestRow test_row Why not to use a reference instead of copy here? If using reference, then no need to replace (copy-over) it later calling something like 'snapshot[row_key] = std::move(test_row)' PS10, Line 571: CHECK_EQ Why not just ASSERT_EQ() ? PS10, Line 602: 20000 nit: may be, create a constant for this timeout and use it here and in all other places in this test? PS10, Line 617: VLOG(1) << "Reinserted " << reinserts.size() << " rows"; Is the 'reinserts' container valid at this point? There is set_reinserts(std::move(reinserts)) call prior to that in case of force_reupdate_missed_deltas -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
