[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11858 ) Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. (03/05) delta_store: support iteration with snap_to_exclude This patch changes the delta stores (DMS and delta files) to respect snap_to_exclude during iteration. The key changes are: - The introduction of the "selection" criteria, a new delta relevancy formula for determining whether a delta applies to a scan with both snap_to_exclude and snap_to_include. The existing "application" criteria was formalized and moved into delta_relevancy.h. There was also a non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both criterias when culling entire delta files. - A new SelectUpdates() method for using the selection criteria on a batch of prepared deltas. SelectUpdates() requires new in-memory state, the creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not to affect regular scans. - Updates to the delta fuzz testing logic to test iterators with two timestamps, and to provide SelectUpdates() coverage. A future patch will modify DeltaApplier to use SelectUpdates for diff scans. Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Reviewed-on: http://gerrit.cloudera.org:8080/11858 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h A src/kudu/tablet/delta_relevancy.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 11 files changed, 473 insertions(+), 92 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
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 5: Code-Review+2 (1 comment) 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 > Eh, I guess, but there's prior art for this naming convention (see Reupdate I know it's disruptive and bikesheddy, we can consider a naming change later perhaps -- 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: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 07 Dec 2018 03:45:10 + Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
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 4: (1 comment) 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 > exclude mutations before the commit timestamp of this snapshot, I think? Eh, I guess, but there's prior art for this naming convention (see ReupdateMissedDeltas in compaction.cc) and the consistency is useful. -- 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: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 27 Nov 2018 00:24:08 + Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
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 4: (2 comments) 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 > Not following you; what does 'before' signify? exclude mutations before the commit timestamp of this snapshot, I think? http://gerrit.cloudera.org:8080/#/c/11858/3/src/kudu/tablet/deltafile.cc@302 PS3, Line 302: IsDeltaRelevantForApply(snap_to_include, > If I'm understanding you correctly, you'd like to explicitly mention UNDO b The comment you added in rev 4 helps, thanks -- 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: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 26 Nov 2018 19:37:42 + Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11858 to look at the new patch set (#4). Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. (03/05) delta_store: support iteration with snap_to_exclude This patch changes the delta stores (DMS and delta files) to respect snap_to_exclude during iteration. The key changes are: - The introduction of the "selection" criteria, a new delta relevancy formula for determining whether a delta applies to a scan with both snap_to_exclude and snap_to_include. The existing "application" criteria was formalized and moved into delta_relevancy.h. There was also a non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both criterias when culling entire delta files. - A new SelectUpdates() method for using the selection criteria on a batch of prepared deltas. SelectUpdates() requires new in-memory state, the creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not to affect regular scans. - Updates to the delta fuzz testing logic to test iterators with two timestamps, and to provide SelectUpdates() coverage. A future patch will modify DeltaApplier to use SelectUpdates for diff scans. Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h A src/kudu/tablet/delta_relevancy.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 11 files changed, 473 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11858/4 -- 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: newpatchset Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
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(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(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:
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11858 ) Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- 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: deleteReviewer Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
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: Verified+1 Overriding Jenkins, the failure was known flake KUDU-2599. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 05:52:02 + Gerrit-HasComments: No
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11858 to look at the new patch set (#3). Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. (03/05) delta_store: support iteration with snap_to_exclude This patch changes the delta stores (DMS and delta files) to respect snap_to_exclude during iteration. The key changes are: - The introduction of the "selection" criteria, a new delta relevancy formula for determining whether a delta applies to a scan with both snap_to_exclude and snap_to_include. The existing "application" criteria was formalized and moved into delta_relevancy.h. There was also a non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both criterias when culling entire delta files. - A new SelectUpdates() method for using the selection criteria on a batch of prepared deltas. SelectUpdates() requires new in-memory state, the creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not to affect regular scans. - Updates to the delta fuzz testing logic to test iterators with two timestamps, and to provide SelectUpdates() coverage. A future patch will modify DeltaApplier to use SelectUpdates for diff scans. Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h A src/kudu/tablet/delta_relevancy.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 11 files changed, 468 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11858/3 -- 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: newpatchset Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11858 to look at the new patch set (#2). Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. (03/05) delta_store: support iteration with snap_to_exclude This patch changes the delta stores (DMS and delta files) to respect snap_to_exclude during iteration. The key changes are: - The introduction of the "selection" criteria, a new delta relevancy formula for determining whether a delta applies to a scan with both snap_to_exclude and snap_to_include. The existing "application" criteria was formalized and moved into delta_relevancy.h. There was also a non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both criterias when culling entire delta files. - A new SelectUpdates() method for using the selection criteria on a batch of prepared deltas. SelectUpdates() requires new in-memory state, the creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not to affect regular scans. - Updates to the delta fuzz testing logic to test iterators with two timestamps, and to provide SelectUpdates() coverage. A future patch will modify DeltaApplier to use SelectUpdates for diff scans. Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h A src/kudu/tablet/delta_relevancy.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 11 files changed, 468 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11858/2 -- 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: newpatchset Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
