Mike Percy has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 18: (12 comments) http://gerrit.cloudera.org:8080/#/c/4363/18//COMMIT_MSG Commit Message: PS18, Line 17: for the op > nit: remove "for the op" here and below Done PS18, Line 17: 1. UpdateStats() for the op returns the maximum potentially gc'able : bytes of undos in the rowset, which is the sum of all undo delta : store sizes up until an initialized one with : max_timestamp > the AHM (ancient history mark). > mention that eventually this will be exact Done PS18, Line 22: deltas > s/deltas/delta stores for the tablet (and remove on the tablet at the end) Done PS18, Line 23: Per rowset it performs a binary search and : initializes undo delta stores until it finds the earliest one with : max_timestamp > AHM > this is no longer true Done PS18, Line 30: To avoid starvation of performance ops, there is a flag that : incorporates some randomness into the scheduler, at the MM level. This : limits the fraction of the time we consider data GC ops higher : priority than performance ops. > mention the flag by name Done PS18, Line 47: This should be safe because we are not actually : modifying any data, we are simply removing references to blocks that : are no longer reachable by new scanners. > mention how you made sure that this wouldn't race with compactions Done http://gerrit.cloudera.org:8080/#/c/4363/18/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: PS18, Line 193: AssertEventually([&] { : ASSERT_GT(tablet->metrics()->undo_delta_block_gc_init_duration->TotalCount(), 0); : ASSERT_GT(tablet->metrics()->undo_delta_block_gc_delete_duration->TotalCount(), 0); : ASSERT_GT(tablet->metrics()->undo_delta_block_gc_perform_duration->TotalCount(), 0); : ASSERT_EQ(0, tablet->metrics()->undo_delta_block_gc_running->value()); : ASSERT_GT(tablet->metrics()->undo_delta_block_gc_bytes_deleted->value(), 0); : ASSERT_EQ(0, tablet->metrics()->undo_delta_block_estimated_retained_bytes->value()); : }); > would be good to add an assertion that actually made sure we're occupying l Done http://gerrit.cloudera.org:8080/#/c/4363/18/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: PS18, Line 178: void DeltaTracker::ValidateDeltaOrder(const std::shared_ptr<DeltaStore>& first, : const std::shared_ptr<DeltaStore>& second, : DeltaType type) { > oh, I see you just made them part of the class. please move the docs to the Done PS18, Line 321: because this function is specified : // only to be used for compactions > thanks for making this clearer. Got a little more specific in this comment and in the header comment. http://gerrit.cloudera.org:8080/#/c/4363/18/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: PS18, Line 178: ValidateDeltaOrder > these are new right? please add some docs Done PS18, Line 181: ValidateDeltasOrdered > these are new right? please add some docs Done http://gerrit.cloudera.org:8080/#/c/4363/18/src/kudu/tablet/tablet_mm_ops.h File src/kudu/tablet/tablet_mm_ops.h: PS18, Line 134: void UpdateStats(MaintenanceOpStats* stats) override; > doc this and perform since they have unusual behavior Done -- To view, visit http://gerrit.cloudera.org:8080/4363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0309bf7acfb6d018860c80f354012c3500da5c68 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
