[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Mike Percy, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11029 to review the following change. Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 146 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/1 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11029 to look at the new patch set (#2). Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 145 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/2 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: 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/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 2: Verified+1 Overriding Jenkins, more Java flakes. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 19:05:34 + Gerrit-HasComments: No
[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11029 to look at the new patch set (#5). Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 145 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/5 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11029 to look at the new patch set (#6). Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 150 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/6 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Mike Percy has removed a vote on this change. Change subject: deltamemstore: support iteration with snap_to_exclude .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 9: Verified+1 Overriding Jenkins; it looks like one of the build machines had a disk failure or something along those lines (compilation failed with an error message indicating an unexpectedly truncated file). -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 27 Aug 2018 17:23:08 + Gerrit-HasComments: No
[kudu-CR] deltamemstore: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 9: Code-Review+1 (7 comments) looks good to me http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc File src/kudu/tablet/deltamemstore-test.cc: http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@142 PS9, Line 142: * nit: move the asterisk next to the type http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@579 PS9, Line 579: TEST_F(TestDeltaMemStore, TestScanSnapToExclude) { Mind adding a comment describing this test? Maybe something like: Test to ensure that scanning between two timestamps returns the difference between the two timestamps. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@585 PS9, Line 585: // Sequence of operations: nit: Consider moving this part of the code down below the definition of ApplyAndCheck() so that this explanatory comment and setup code can easily fit on the same screen as the main assertions at the bottom of this test. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@591 PS9, Line 591: snaps.emplace_back(mvcc_); nit: to make this easier to verify by inspection, mind adding // snaps[0] to the end of this line? and similar for other snapshots taken on the nearby lines below http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@592 PS9, Line 592: 0 nit: mind adding a const rowid_t kRowId = 0 up above so we can use it throughout this test? http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@600 PS9, Line 600: auto ApplyAndCheck = [&](const MvccSnapshot& exclude, Please add a doc comment along these lines to this lambda: // Apply mutations according to the specified time range to row 0 and assert // that the specified expected value and liveness of the row are satisfied. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@633 PS9, Line 633: false nit: mind changing this to /*is_row_live=*/ false ? Or maybe use an enum but I think the comment is less hassle. Similar below -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 27 Aug 2018 18:04:07 + Gerrit-HasComments: Yes
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc File src/kudu/tablet/deltamemstore-test.cc: http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@142 PS9, Line 142: * > nit: move the asterisk next to the type Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@579 PS9, Line 579: TEST_F(TestDeltaMemStore, TestScanSnapToExclude) { > Mind adding a comment describing this test? Maybe something like: Test to e Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@585 PS9, Line 585: // Sequence of operations: > nit: Consider moving this part of the code down below the definition of App Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@591 PS9, Line 591: snaps.emplace_back(mvcc_); > nit: to make this easier to verify by inspection, mind adding // snaps[0] t Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@592 PS9, Line 592: 0 > nit: mind adding a const rowid_t kRowId = 0 up above so we can use it throu Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@600 PS9, Line 600: auto ApplyAndCheck = [&](const MvccSnapshot& exclude, > Please add a doc comment along these lines to this lambda: Done http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@633 PS9, Line 633: false > nit: mind changing this to /*is_row_live=*/ false ? Or maybe use an enum bu Gonna skip this because a follow-on patch replaces this with a tri-state enum. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 22:02:28 + Gerrit-HasComments: Yes
[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11029 to look at the new patch set (#10). Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 154 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/10 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 10: Verified+1 Overriding Jenkins, unrelated ASAN failure. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 00:05:20 + Gerrit-HasComments: No
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: 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/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 11: Code-Review-2 Don't merge; I've found an issue with ApplyUpdates() skipping "too old" updates that it shouldn't. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 11 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 19:33:11 + Gerrit-HasComments: No
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Abandoned This ended up being more complicated and I wrote a new series of changes to do it. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 11 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon