Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11858 )
Change subject: (03/05) delta_store: support iteration with snap_to_exclude ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/11858/2/src/kudu/tablet/delta_iterator_merger.cc File src/kudu/tablet/delta_iterator_merger.cc: http://gerrit.cloudera.org:8080/#/c/11858/2/src/kudu/tablet/delta_iterator_merger.cc@83 PS2, Line 83: & > style nit on the & Sure. I resisted the urge to do this across the file. http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/delta_relevancy.h File src/kudu/tablet/delta_relevancy.h: http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/delta_relevancy.h@71 PS3, Line 71: ts > nit: how about delta_ts here and below, instead of just ts, since the snaps Done http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/delta_relevancy.h@74 PS3, Line 74: template<> : inline bool IsDeltaRelevantForApply<REDO>(const MvccSnapshot& snap, : const Timestamp& ts, : bool* finished_row) { : *finished_row = false; : if (!snap.IsCommitted(ts)) { : if (!snap.MayHaveCommittedTransactionsAtOrAfter(ts)) { : // REDO deltas are sorted first in ascending row ordinal order, then in : // ascending timestamp order. Thus, if we know that there are no more : // committed transactions whose timestamps are >= 'ts', we know that any : // future deltas belonging to this row aren't relevant (as per the apply : // criteria, REDOs are relevant if they are committed in the snapshot), : // and we can skip to the next row. : *finished_row = true; : } : return false; : } : return true; : } > I'd slightly prefer to drop a level of nesting to write this as: Done http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/delta_relevancy.h@94 PS3, Line 94: template<> : inline bool IsDeltaRelevantForApply<UNDO>(const MvccSnapshot& snap, : const Timestamp& ts, : bool* finished_row) { : *finished_row = false; : if (snap.IsCommitted(ts)) { : if (!snap.MayHaveUncommittedTransactionsAtOrBefore(ts)) { : // UNDO deltas are sorted first in ascending row ordinal order, then in : // descending timestamp order. Thus, if we know that there are no more : // uncommitted transactions whose timestamps are <= 'ts', we know that any : // future deltas belonging to this row aren't relevant (as per the apply : // criteria, UNDOs are relevant if they are uncommitted in the snapshot), : // and we can skip to the next row. : *finished_row = true; : } : return false; : } : return true; : } > Same as the above, how about: Done http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/deltafile.cc@290 PS3, Line 290: snap_to_exclude > shower thought: should we call this snap_exclude_before? Not following you; what does 'before' signify? http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/deltafile.cc@302 PS3, Line 302: if (snap_to_exclude) { > I we should add a comment justifying the OR logic (|=), since it takes some If I'm understanding you correctly, you'd like to explicitly mention UNDO because the select criteria is not a strict subset of the UNDO apply criteria (unlike REDO), and you think that's an important distinction that the reader should know. I disagree; as you can see, there's no special treatment here to avoid calling IsDeltaRelevantForApply when dealing with UNDOs, and even though that may be a useful microoptimization, I didn't want readers of DeltaFileReader to have to burden themselves with that piece of information, which they can extract from delta_relevancy.h. The general point stands on its own merit: in order to decide if a DeltaFileReader is relevant for a particular pair of snapshots, we need to consider both the apply criteria and the select criteria. If either one is relevant, the DeltaFileReader is relevant. I'll try to distill this into a useful comment. -- To view, visit http://gerrit.cloudera.org:8080/11858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 21 Nov 2018 23:35:36 +0000 Gerrit-HasComments: Yes
