Mike Percy has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files ......................................................................
Patch Set 19: (10 comments) http://gerrit.cloudera.org:8080/#/c/4995/19/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: Line 556: spurious newline Line 618: ts = ts == Timestamp::kInvalidTimestamp ? Timestamp(clock_->GetCurrentTime()) : ts; how about: if (ts == Timestamp::kInvalidTimestamp) ts = Timestamp(clock_->GetCurrentTime()); PS19, Line 670: In the end make sure that the output is correct. How about: Repeatedly merge all the generated RowSets until we are left with a single RowSet, then make sure that its history matches our expected history. Line 699: for (int j = 0; j < num_rowsets_in_layer; ++j) { Mind adding a comment about the purpose of this "layers" logic? Line 715: // For odd rows, update them and delete them in the drs Is there any particular reason why the odd rows in the DRS are updated with null values in AddExpectedUpdate() ? Also, need punctuation in the comment. PS19, Line 717: row_idx++ Increment row_idx on a separate line for readability Line 724: // For the next layer remove one rowset worth or rows at random needs period PS19, Line 724: or of Line 727: row_ids.erase(row_ids.begin() + to_remove); expensive operation, isn't it? Line 762: // Merge between 2 and 4 row sets and here -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
