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<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 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 <[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: Mon, 17 Sep 2018 15:49:10 +0000 Gerrit-HasComments: Yes
