David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 18: (13 comments) http://gerrit.cloudera.org:8080/#/c/4363/18//COMMIT_MSG Commit Message: PS18, Line 7: KUDU-1601. Delete ancient UNDO delta blocks in the background As we discussed it would be important to run this on dist-test and have the link here. Maybe even report real cluster results, if you have the time. PS18, Line 17: for the op nit: remove "for the op" here and below 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 PS18, Line 22: deltas s/deltas/delta stores for the tablet (and remove on the tablet at the end) 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 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 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 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 less space on disk. Anyway at all this is feasible easily? 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 header. PS18, Line 321: because this function is specified : // only to be used for compactions thanks for making this clearer. you mean this block right? the undos gc code also calls this funtion. 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 PS18, Line 181: ValidateDeltasOrdered these are new right? please add some docs 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 -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
