David Ribeiro Alves 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 Done Line 618: ts = ts == Timestamp::kInvalidTimestamp ? Timestamp(clock_->GetCurrentTime()) : ts; > how about: Done 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 wit Done Line 699: for (int j = 0; j < num_rowsets_in_layer; ++j) { > Mind adding a comment about the purpose of this "layers" logic? I did to the header. having overlapping layers is so that we have stuff to compact together. having one less row set per layer is so that we'll likely always have rows that are unique in a compaction input which is more "normal" than the artificial scenario of having all rowsets but the bottom one have only duplicated rows. 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 no just the way this test worked previously (look at insert/update row). Done PS19, Line 717: row_idx++ > Increment row_idx on a separate line for readability Done Line 724: // For the next layer remove one rowset worth or rows at random > needs period Done PS19, Line 724: or > of Done Line 727: row_ids.erase(row_ids.begin() + to_remove); > expensive operation, isn't it? this test runs in 374 ms total, my guess is that 90% of that time is spent on writes, i don't think that's an issue. Line 762: // Merge between 2 and 4 row sets > and here Done -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
