Mike Percy 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 I tried again, LMK if this is clearer. PS16, Line 363: Timestamp::kInvalidTimestamp > my observation about making the timestamp kInitialTimestamp was that you wo In the case of this API I don't think we need to support this so I will remove it. That's not what I want with the Init() API, though. If max_timestamp > AHM then it would short circuit, whereas I would like it to Init() everything if the AHM is invalid. 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 The main point is we are calling Init() to try and optimize Estimate(). But based on our conversation on Slack I'll remove this early optimization for now. PS16, Line 387: if (ancient_history_mark != Timestamp::kInvalidTimestamp) { > it's kind of weird that if timestamp is kInvalidTimestmap this doesn't actu The purpose of this search was to find the undo at the boundary of the AHM, so if there is no AHM it doesn't make sense to do the search. But I've removed this code so now it's a moot point. In the code block below, if the AHM is invalid then it actually Init()s everything, which I rely on in tests. PS16, Line 391: deadline.Initialized() > can we skip the Initialized and just use MonoTime::max() when we don't want removed PS16, Line 407: deadline.Initialized() > same This is useful because I also want to provide the option to specify an unlimited budget at the Tablet level (it's used by tests) and it's more natural to convert an uninitialized budget to an uninitialized deadline vs. trying to ensure that a max budget ends up being coerced into a max deadline. 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, wh I think it makes sense that you can't delete ancient undos if you don't have the AHM to determine whether they are ancient. However this would probably make a better DCHECK since I think it's a programming error. -- 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: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
