Mike Percy has posted comments on this change. Change subject: Don't output unobservable rows from the MemRowset ......................................................................
Patch Set 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: Line 618: FlushMRSAndReopenNoRoll(*mrs, schema_, &rs2); Is it important that we do not roll? http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 113: && insertion_timestamp.CompareTo(input_row.redo_head->timestamp()) == 0)) { Consider writing this like: if (PREDICT_FALSE(input_row.redo_head != nullptr && input_row.redo_head->timestamp() == insertion_timestamp)) { I also wonder if PREDICT_FALSE is really needed here. Line 114: // Get the latest mutation nit: punctuation Line 117: if (latest->changelist().is_delete() similarly, consider writing this like: if (latest->changelist().is_delete() && latest->timestamp() == insertion_timestamp) { Line 134: if (PREDICT_FALSE(num_in_block != next_row_index)) { Maybe better written as: if (PREDICT_FALSE(next_row_index < num_in_block)) { -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[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
