Mike Percy 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: (7 comments) looks great 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 & 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 snapshot also represents some kind of timestamp or set of timestamps 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: template<> inline bool IsDeltaRelevantForApply<REDO>(const MvccSnapshot& snap, const Timestamp& ts, bool* finished_row) { *finished_row = false; if (snap.IsCommitted(ts)) { return true; } 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; } 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: template<> inline bool IsDeltaRelevantForApply<UNDO>(const MvccSnapshot& snap, const Timestamp& ts, bool* finished_row) { *finished_row = false; if (!snap.IsCommitted(ts)) { return true; } 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; } 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? http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/deltafile.cc@291 PS3, Line 291: snap_to_include shower thought: should we call this snap_include_before? 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 effort to derive it from first principles. Maybe something like: // In addition to apply criteria, which must apply UNDO records that occur after the end of snap_to_include timestamp, for range-select we must also include any kind of delta that occurred between the two specified snapshot timestamps. -- 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 18:55:36 +0000 Gerrit-HasComments: Yes
