You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/11/02 05:14:37 UTC
[kudu-CR] delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/11858
to review the following change.
Change subject: delta_store: support iteration with snap_to_exclude
......................................................................
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/1
--
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: newchange
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 27 Nov 2018 00:24:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Dec 2018 03:45:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Nov 2018 05:52:02 +0000
Gerrit-HasComments: No
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:35:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 21 Nov 2018 18:55:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
---
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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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<REDO>(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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 26 Nov 2018 19:37:42 +0000
Gerrit-HasComments: Yes