Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15506 )
Change subject: tablet: plumb delta stats into delta compaction outputs ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/15506/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15506/3//COMMIT_MSG@15 PS3, Line 15: I considered also plumbing stats into merge compactions, but opted not : to > Ah, makes sense; you're referring to the new stats built during the merge c Done http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_compaction.cc@135 PS3, Line 135: unique_ptr<DeltaStats> redo_stats(new DeltaStats); : unique_ptr<DeltaStats> undo_stats(new DeltaStats); > Aren't DeltaStats trivially movable (i.e. doesn't the compiler generate mov It does, though unless we start plumbing boost::optional<DeltaStats> into DeltaFileReader, I don't think there's much avoiding this allocation. I'll leave a TODO, since that plumbing seems like a bit much. FWIW a trivial copy is still a memcpy of a bunch of bytes, vs a pointer move with unique_ptr, though the heap allocation is certainly not great. http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_compaction.cc@421 PS3, Line 421: vector<unique_ptr<DeltaStats>> redo_stats; > Also hoping we could avoid these allocations by moving DeltaStats instances Ack http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_tracker.h@42 PS3, Line 42: class DeltaStats; > warning: declaration 'DeltaStats' is never referenced, but a declaration wi Done http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/delta_tracker.cc@108 PS3, Line 108: DCHECK(stats.empty() || blocks.size() == stats.size()) << : Substitute("Unexpected number of stats: expected 0 or $0, got $1", : blocks.size(), stats.size()); > This suggests that there should be just a single vector of a struct. Done http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/tablet_history_gc-test.cc File src/kudu/tablet/tablet_history_gc-test.cc: http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/tablet_history_gc-test.cc@101 PS3, Line 101: erroring out if it failed or our : // metrics don't make sense. > Well, we ASSERT fail in this case rather than "erroring out". Done http://gerrit.cloudera.org:8080/#/c/15506/3/src/kudu/tablet/tablet_history_gc-test.cc@124 PS3, Line 124: void DeleteOriginalRows(int64_t num_rowsets, int64_t rows_per_rowset, bool flush_dms); > warning: function 'kudu::tablet::TabletHistoryGcTest::DeleteOriginalRows' h Done -- To view, visit http://gerrit.cloudera.org:8080/15506 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2f28fb2905ddcc007c88ab80ae2185587400f0 Gerrit-Change-Number: 15506 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 21 Mar 2020 01:24:09 +0000 Gerrit-HasComments: Yes
