[kudu-CR] memrowset: 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/10926 to review the following change. Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 144 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/1 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: 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/10926 to look at the new patch set (#2). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 145 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/2 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 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] memrowset: 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/10926 to look at the new patch set (#3). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 145 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/3 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 3 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] memrowset: 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/10926 to look at the new patch set (#4). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 144 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/4 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 4 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] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 5: Verified+1 Overriding Jenkins, unrelated test failure. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 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 Gerrit-Comment-Date: Wed, 18 Jul 2018 02:23:09 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: 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/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 5 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] memrowset: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 5: (3 comments) Nice test! http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.h@553 PS5, Line 553: // The 'unset_in_sel_vector' parameter will be overwritten if: : // - At least one mutation was relevant (will be set to false). : // - The last mutation walked was a DELETE (will be set to true). How about we call this 'selected' and reverse the negative semantics of this in-out param? http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc@507 PS5, Line 507: it : // means the insertion was outside this iterator's time range I think for the MRS this might not be an issue, but for DRS it likely may mean that snap_to_exclude is earlier than the ancient history mark and the corresponding insert UNDO record has been GCed, because base data does not have an insertion timestamp. I am not sure if that's relevant or not, because I haven't yet spent enough time thinking about how the DRS changes are going to interact with the APIs you're building in this patch series. http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc@608 PS5, Line 608: *unset_in_sel_vector = true; I'm assuming we're going to have to handle this differently when we get to the part about adding a 'deleted' virtual column. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 19 Jul 2018 18:15:38 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc@507 PS5, Line 507: it : // means the insertion was outside this iterator's time range > I think for the MRS this might not be an issue, but for DRS it likely may m After letting this observation marinate for a few minutes, I think it's expected behavior, because a request for a diff scan with snap_to_exclude < the AHM has a good chance of being impossible to respond to correctly. So it should be rejected, just like we reject scans with snap_to_include < AHM. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 19 Jul 2018 18:35:45 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.h@553 PS5, Line 553: // The 'unset_in_sel_vector' parameter will be overwritten if: : // - At least one mutation was relevant (will be set to false). : // - The last mutation walked was a DELETE (will be set to true). > How about we call this 'selected' and reverse the negative semantics of thi Gonna punt on this since the very next patch changes these parameters anyway. http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc@507 PS5, Line 507: it : // means the insertion was outside this iterator's time range > After letting this observation marinate for a few minutes, I think it's exp As you point out, there's a new interaction with the AHM here that should be more or less equivalent to the existing one with snap_to_include. http://gerrit.cloudera.org:8080/#/c/10926/5/src/kudu/tablet/memrowset.cc@608 PS5, Line 608: *unset_in_sel_vector = true; > I'm assuming we're going to have to handle this differently when we get to Indeed, see one of the following patches. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 19 Jul 2018 18:46:12 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: 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/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 6 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] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 6: Verified+1 Overriding Jenkins, unrelated test failures. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 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 Gerrit-Comment-Date: Thu, 19 Jul 2018 21:39:27 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 19 Jul 2018 22:05:58 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 6: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc@586 PS6, Line 586: // @1 Nit: Zero base these @# comments so it lines up with the snaps indices below. http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc@620 PS6, Line 620: // Captures zero rows; a snapshot range [x, x) does not include anything. Nit: What happens if exclude is beyond include? -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 20 Jul 2018 19:23:50 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc@586 PS6, Line 586: // @1 > Nit: Zero base these @# comments so it lines up with the snaps indices belo The timestamp output isn't zero-based though: @1: row (string key="row", uint32 val=0) mutations=[@2(SET val=1), @3(DELETE), @4(REINSERT val=2)] http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc@620 PS6, Line 620: // Captures zero rows; a snapshot range [x, x) does not include anything. > Nit: What happens if exclude is beyond include? That's illegal, though there's nothing enforcing that today. I'd add enforcement but it's tricky to do (because MvccSnapshot isn't just one timestamp, it's actually one "lower bound", one "upper bound", and, optionally, several points in between. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sun, 22 Jul 2018 23:42:43 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10926/6/src/kudu/tablet/memrowset-test.cc@620 PS6, Line 620: // Captures zero rows; a snapshot range [x, x) does not include anything. > That's illegal, though there's nothing enforcing that today. I'd add enforc That makes sense. Given this is internal I don't think that edge case enforcement is required. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 23 Jul 2018 18:28:43 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10926 to look at the new patch set (#7). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 144 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/7 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 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] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 7: Verified+1 Overriding Jenkins, unrelated Java test failures. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 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 18:00:45 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: 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/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 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] memrowset: support iteration with snap to exclude
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514
PS7, Line 514: bool unset_in_sel_vector
this name is making me a little confused, because it can change again after
it's set here. For the purposes of code clarity, do you think this could be
something like:
// Assume the row should be included unless we determine otherwise below.
bool unset_in_sel_vector = false;
bool insertion_excluded_by_snaps = ...;
if (insertion_excluded_by_snaps || opts_.snap_to_include.IsCommitted(...)) {
// maybe use an enum to take the out-param of ApplyMutations, like
NONE_APPLIED, APPLIED_AND_DELETED, APPLIED_AND_PRESENT
ApplyMutations(...);
unset_in_sel_vector = mut_status == APPLIED_AND_DELETED || (mut_status ==
NONE_APPLIED && insertion_excluded_by_snap);
} else {
unset_in_sel_vector = true;
}
I think the code is equivalent, just trying to find something that's a little
easier to follow here without trying to track this tri-state of
unset_in_sel_vector through multiple functions where it might get set or left
alone in various places
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517
PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) {
: state_ = kFinished;
: break;
I wonder if this actually belongs outside the if condition? Even if we exclude
some row due to MVCC, if we see that we've reached a key above our scan bounds,
we can stop at the point instead of continuing until we find a non-excluded row
(which would necesarily be out of bounds as well)
(it seems like it should have been like this before as well, not new in your
change, but maybe would make this look a little less complex)
--
To view, visit http://gerrit.cloudera.org:8080/10926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce
Gerrit-Change-Number: 10926
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:36:09 +
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514
PS7, Line 514: bool unset_in_sel_vector
> this name is making me a little confused, because it can change again after
Yeah, I agree that mutating unset_in_sel_vector inside
ApplyMutationsToProjectedRow makes this harder to understand.
I think a descriptive enum as an out parameter is a nice touch. I'll make the
change here knowing that the net result is more information than necessary for
this patch, but useful for the next one.
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517
PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) {
: state_ = kFinished;
: break;
> I wonder if this actually belongs outside the if condition? Even if we excl
Makes sense. I'll pull this check out of the outer if.
--
To view, visit http://gerrit.cloudera.org:8080/10926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce
Gerrit-Change-Number: 10926
Gerrit-PatchSet: 7
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, 25 Jul 2018 02:30:30 +
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10926 to look at the new patch set (#8). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. I've also pulled the "exceeded the iterator's scan bounds" short-circuit out of the MVCC checks. If it were true for a row excluded by MVCC, it'd be true for all subsequent rows too. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 173 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/8 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 8 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] memrowset: support iteration with snap to exclude
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 8: Code-Review+1 much easier for me to follow now, thanks! -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 8 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, 25 Jul 2018 02:46:10 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 8: Code-Review+2 I'm good with this if Grant is. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 8 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, 25 Jul 2018 20:13:06 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. I've also pulled the "exceeded the iterator's scan bounds" short-circuit out of the MVCC checks. If it were true for a row excluded by MVCC, it'd be true for all subsequent rows too. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Reviewed-on: http://gerrit.cloudera.org:8080/10926 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 173 insertions(+), 40 deletions(-) Approvals: Todd Lipcon: Looks good to me, but someone else must approve Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 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
