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<ColumnUpdate> UpdatesForColumn; : std::vector<UpdatesForColumn> updates_by_col_; : std::deque<rowid_t> deleted_; : : // State when prepared_for_ == PREPARED_FOR_COLLECT : // ------------------------------------------------------------ : struct PreparedDelta { : DeltaKey key; : Slice val; : }; : std::deque<PreparedDelta> 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 20 Sep 2018 02:23:19 +0000 Gerrit-HasComments: Yes
