Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11395 )
Change subject: KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h@293 PS3, Line 293: MaybeProcessRowChange How about rename and slightly modify to be a bit more explicit about the semantics of the API? I know this is a private method but due to the nature of the control flow (it's tricky) I think it's worth explaining how to use this in more detail. // Checks whether we are done processing a row's deltas by comparing 'cur_row_idx' to 'last_added_idx_'. If they are both set and not equal, we assume we are done with the previous row, and we will attempt to convert the row's latest deletion state into a saved deletion or reinsertion if needed. // If 'cur_row_idx' is not set, we will process the delta state row the row in 'last_added_idx_' if it is set. That method should be called when we are done processing a batch of deltas, such as from Finish(). MaybeProcessPreviousRowChange(boost::optional<rowid_t> cur_row_idx) http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h@305 PS3, Line 305: rowid_t How about boost::optional<rowid_t> to avoid having to check for the sentinel value numeric_limits<rowid_t>::max()? http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@59 PS3, Line 59: with a higher at the given or a higher http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@60 PS3, Line 60: false; it's set to true this looks reversed to me http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@165 PS3, Line 165: MaybeProcessRowChange(key.row_idx()); It seems like this should be done after the IsDeltaRelevant() check http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@178 PS3, Line 178: if (!Traits::kAllowReinserts && decoder.is_reinsert()) { nit: PREDICT_FALSE? http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@182 PS3, Line 182: UpdateDeletionState(decoder.get_type()); Can we call MaybeProcessRowChange() from within UpdateDeletionState() ? http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@223 PS3, Line 223: last_added_idx_ = key.row_idx(); Can we call UpdateDeletionState() down here at the end? It would be better to update last_added_idx_ as part of the UpdateDeletionState call to manage that state all in one place. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@337 PS3, Line 337: last_added_idx_ != MathLimits<rowid_t>::kMax I'm not a big fan of this... see my comment in the header file about using boost::optional for last_added_idx_ instead. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltafile.cc@623 PS3, Line 623: visitor AddDelta() call http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltamemstore.cc@248 PS3, Line 248: // TODO(adar): is this correct? I think this comment should be: // Skip the remaining timestamps for this row. By the way, this looks right to me, but it seems like we should put in test coverage for this optimization to be sure it does what we think it does. -- To view, visit http://gerrit.cloudera.org:8080/11395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5 Gerrit-Change-Number: 11395 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 14 Sep 2018 18:43:46 +0000 Gerrit-HasComments: Yes
