David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/4363/16/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: PS16, Line 301: because this function is specified : // only to be used for compactions. again not totally clear. Can you elaborate on why this being just used for compactions is relevant? PS16, Line 363: Timestamp::kInvalidTimestamp my observation about making the timestamp kInitialTimestamp was that you wouldn't need this extra if condition, it would always compare to less than the AHM PS16, Line 383: // First, find the boundary of where we should stop initializing. : // We use binary search to find the oldest delta store with max_timestamp >= AHM. : // Using binary search here allows us to get a better estimate from : // EstimateBytesInPotentiallyAncientUndoDeltas() later. I'm not following. you use binary search but then ignore what you found or am i'm missing something? what's the trick here? if it's not extremely useful I'd suggest dropping this and going for simplicity at least postpone to a follow up patch where we can show this is much better. PS16, Line 387: if (ancient_history_mark != Timestamp::kInvalidTimestamp) { it's kind of weird that if timestamp is kInvalidTimestmap this doesn't actually init anything. In which cases is kInvalidTimestamp used? is it the default value for the the AHM? PS16, Line 391: deadline.Initialized() can we skip the Initialized and just use MonoTime::max() when we don't want a deadline? PS16, Line 407: deadline.Initialized() same PS16, Line 431: if (ancient_history_mark == Timestamp::kInvalidTimestamp) { : return Status::InvalidArgument("ancient_history_mark must not be an invalid timestamp"); : } so in the method above we silently do nothing on this value for the ahm, while here we return an error status. -- 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: 16 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: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
