David Ribeiro Alves has posted comments on this change. Change subject: [Timestamp] use 'operator<' instead of ComesBefore ......................................................................
Patch Set 3: (4 comments) lgtm, but please remove the CompactionInputRow stuff http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: PS3, Line 474: : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : same PS3, Line 350: if (smallest < state->next()) { same PS3, Line 565: namespace { : void AdvanceToLastInList(const Mutation** m) { : const Mutation* next; : while ((next = (*m)->acquire_next()) != nullptr) { : *m = next; : } : } : } // anonymous namespace : : // Compare the mutations of two duplicated rows. : // Return 'true' if latest_version(left) < latest_version(right) : bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs) { : const Mutation* left_latest = lhs.redo_head; : const Mutation* right_latest = rhs.redo_head; : if (right_latest == nullptr) { : DCHECK(left_latest != nullptr); : return true; : } : if (left_latest == nullptr) { : // left must still be alive : DCHECK(right_latest != nullptr); : return false; : } : : // Duplicated rows have disjoint histories, we don't need to get the latest : // mutation, the first one should be enough for the sake of determining the most recent : // row, but in debug mode do get the latest to make sure one of the rows is a ghost. : const bool ret = left_latest->timestamp() < right_latest->timestamp(); : #ifndef NDEBUG : AdvanceToLastInList(&left_latest); : AdvanceToLastInList(&right_latest); : if (left_latest->timestamp() != right_latest->timestamp()) { : // If in fact both rows were deleted at the same time, this is OK -- we could : // have a case like TestRandomAccess.TestFuzz3, in which a single batch : // DELETED from the DRS, INSERTed into MRS, and DELETED from MRS. In that case, : // the timestamp of the last REDO will be the same and we can pick whichever : // we like. : const bool debug_ret = left_latest->timestamp() < right_latest->timestamp(); : CHECK_EQ(ret, debug_ret); : } : #endif : return ret; : } same http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: PS3, Line 167: bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs); this seems out of place in this patch (the commit message only mentions Timestamp) and actually goes against the reinsert stuff I have in-flight where I completely change the comparison between two CompactionInputRows, mind removing this from this patch? -- To view, visit http://gerrit.cloudera.org:8080/5096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
