[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 12: Code-Review+2 Carrying forward David's +2. -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 12 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Oct 2018 23:58:07 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code. The one exception is that I replaced prepared_idx_ and prepared_count_ with state that I found easier to understand. No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Reviewed-on: http://gerrit.cloudera.org:8080/11394 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 423 insertions(+), 277 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 13 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 11: Code-Review+2 Carrying forward +2 from David. -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 11 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Oct 2018 23:19:47 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 10: Verified+1 Overriding Jenkins, unrelated test flake. -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Oct 2018 03:16:50 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Oct 2018 16:30:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 09 Oct 2018 16:29:41 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 04 Oct 2018 03:52:40 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 29 Sep 2018 16:16:54 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 24 Sep 2018 15:06:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@133 PS4, Line 133: PreparedDeltas > This is an interesting idea, and I spent some time thinking about it. In th k, thanks for looking into it -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 24 Sep 2018 15:06:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11394 to look at the new patch set (#5). Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code. The one exception is that I replaced prepared_idx_ and prepared_count_ with state that I found easier to understand. No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 423 insertions(+), 277 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/5 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11394 )
Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into
DeltaPreparer
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h
File src/kudu/tablet/delta_store.h:
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@133
PS4, Line 133: PreparedDeltas
> would it make sense to have two classes instead of one? e.g. PreparedDeltas
This is an interesting idea, and I spent some time thinking about it. In the
end, I think the answer is 'no' for the following reasons:
1. There is some overlap between the two classes: opts_, cur_prepared_idx_, and
prev_prepared_idx_. I can duplicate them, but I have an unpublished patch that
adds PREPARE_FOR_SELECT with its own state, so that would mean duplicating the
common state three times. I can encapsulate it into a common "PreparedDeltas"
class, but now we're talking about a fair amount of class complexity for this
platonic ideal of isolation.
2. I also have an unpublished patch that converts PrepareFlag into a bitfield
so that we can PREPARE_FOR_APPLY and PREPARE_FOR_SELECT when doing a backup
scan, to avoid setting up more state than necessary in the regular scan case.
This means that a DeltaIterator would need to maintain all three of the above
PreparedDeltasForFoo classes rather than just one at a time, and fan-out
Start/AddDelta/Finish methods accordingly.
So I agree that it can be done, but I worry that the overhead of doing it well
outweighs the overhead of keeping them combined.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@279
PS4, Line 279: // Whether there are any prepared blocks.
> might be a good place to clearly explain the distinction between PREPARE_FO
Done
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@287
PS4, Line 287: // State when prepared_for_ == PREPARED_FOR_APPLY
: //
: struct ColumnUpdate {
: rowid_t row_id;
: void* new_val_ptr;
: uint8_t new_val_buf[16];
: };
: typedef std::deque UpdatesForColumn;
: std::vector updates_by_col_;
: std::deque deleted_;
:
: // State when prepared_for_ == PREPARED_FOR_COLLECT
: //
: struct PreparedDelta {
: DeltaKey key;
: Slice val;
: };
: std::deque prepared_deltas_;
> this separation of state makes lean more towards splitting the classes. how
See above.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@323
PS4, Line 323: enum { ITERATE_OVER_ALL_ROWS = 0 };
> move this enum before the method comment? since you're mentioning what it s
I didn't even notice it was used in DebugDumpDeltaIterator, so yeah, gotta move
this up.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc
File src/kudu/tablet/delta_store.cc:
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc@103
PS4, Line 103: DeltaKey
> nit, pass by reference?
Done
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc@173
PS4, Line 173: Status
> nit missing blank line above
Done
--
To view, visit http://gerrit.cloudera.org:8080/11394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5
Gerrit-Change-Number: 11394
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 20 Sep 2018 02:23:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
David Ribeiro Alves has posted comments on this change. (
http://gerrit.cloudera.org:8080/11394 )
Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into
DeltaPreparer
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h
File src/kudu/tablet/delta_store.h:
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@133
PS4, Line 133: PreparedDeltas
would it make sense to have two classes instead of one? e.g.
PreparedDeltasForApply and PreparedDeltasForCollect, where each of them would
only have the corresponding methods? as far as I can see the methods are very
different and non-overlapping.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@279
PS4, Line 279: // Whether there are any prepared blocks.
might be a good place to clearly explain the distinction between
PREPARE_FOR_APPLY and PREPARE_FOR_COLLECT in terms of what it does.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@287
PS4, Line 287: // State when prepared_for_ == PREPARED_FOR_APPLY
: //
: struct ColumnUpdate {
: rowid_t row_id;
: void* new_val_ptr;
: uint8_t new_val_buf[16];
: };
: typedef std::deque UpdatesForColumn;
: std::vector updates_by_col_;
: std::deque deleted_;
:
: // State when prepared_for_ == PREPARED_FOR_COLLECT
: //
: struct PreparedDelta {
: DeltaKey key;
: Slice val;
: };
: std::deque prepared_deltas_;
this separation of state makes lean more towards splitting the classes. how
feasible would that be in your opinion?
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.h@323
PS4, Line 323: enum { ITERATE_OVER_ALL_ROWS = 0 };
move this enum before the method comment? since you're mentioning what it
stands for right after likely you don't need to comment the enum itself.
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc
File src/kudu/tablet/delta_store.cc:
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc@103
PS4, Line 103: DeltaKey
nit, pass by reference?
http://gerrit.cloudera.org:8080/#/c/11394/4/src/kudu/tablet/delta_store.cc@173
PS4, Line 173: Status
nit missing blank line above
--
To view, visit http://gerrit.cloudera.org:8080/11394
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5
Gerrit-Change-Number: 11394
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 17 Sep 2018 15:49:10 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11394/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11394/2/src/kudu/tablet/delta_store.h@219 PS2, Line 219: // Encapsulates all logic and responsibility related to "delta preparation"; > Mind adding a little more color around what this class does? Done -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 15 Sep 2018 01:54:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11394 to look at the new patch set (#4). Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code. The one exception is that I replaced prepared_idx_ and prepared_count_ with state that I found easier to understand. No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 406 insertions(+), 276 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/4 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11394 ) Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. Patch Set 3: Code-Review+2 (1 comment) Changes look good to me http://gerrit.cloudera.org:8080/#/c/11394/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11394/2/src/kudu/tablet/delta_store.h@219 PS2, Line 219: // PreparedDeltas implementation suitable for composing inside a DeltaIterator. Mind adding a little more color around what this class does? -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 14 Sep 2018 18:43:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11394 to look at the new patch set (#3). Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code, even when I didn't really understand it (the prepared_idx_ and prepared_count_ state tracking gave me a lot of grief). No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 391 insertions(+), 276 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/3 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11394 to look at the new patch set (#2). Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code, even when I didn't really understand it (the prepared_idx_ and prepared_count_ state tracking gave me a lot of grief). No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 397 insertions(+), 272 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/2 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer
Hello Mike Percy, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11394 to review the following change. Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer .. KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer To address KUDU-686, we're going to repurpose DMSIterator's PrepareBatch() machinery and associated in-memory state for use in the DeltaFileIterator. Doing so obviates the need for a "multi-pass" approach to ApplyUpdates() because DeltaFileIterator will no longer be performing any I/O or decoding there, having done all of it in PrepareBatch(). This patch lays the groundwork by refactoring the guts of DMSIterator into the new DeltaPreparer class. DMSIterator will continue to concern itself with CBTree iteration, but will delegate the delta preparation and service to DeltaPreparer. In performing this refactor, I tried to be as faithful as possible to the original code, even when I didn't really understand it (the prepared_idx_ and prepared_count_ state tracking gave me a lot of grief). No new tests; I figured there was enough test coverage of DMSIterator, and testing DeltaPreparer directly seemed like it'd be low bang for the buck. Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.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 8 files changed, 397 insertions(+), 273 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/11394/1 -- To view, visit http://gerrit.cloudera.org:8080/11394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5 Gerrit-Change-Number: 11394 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
