Todd Lipcon has posted comments on this change. Change subject: KUDU-1345. Fix case in which hybrid clock can run backwards ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/2266/3/src/kudu/server/hybrid_clock-test.cc File src/kudu/server/hybrid_clock-test.cc: Line 76: // Test that, if the event arrival rate is greater than the maximum resolution of the > not sure "event" is meaningful here. maybe rephrase to "timestamp retrieval Done Line 107: Timestamp now = clock->Now(); > Maybe I'm misunderstanding how this works, but couldn't we assert more stro Done http://gerrit.cloudera.org:8080/#/c/2266/3/src/kudu/server/hybrid_clock.cc File src/kudu/server/hybrid_clock.cc: Line 232: // If the physical time from the local node is higher than our last-returned > s/local node/system clock or remove altogether Done Line 234: uint64_t candidate_phys_timestamp = now_usec << kBitsToShift; > Why not build a Timestamp immediately and use CompareTo() below? then we'd just have to wrap next_timestamp_ for the comparison and unwrap the Timestamp again on line 236. If we made the member variable next_timestamp_ into a Timestamp then the post-increment would become ugly (unless we went with operator overloading, which is disallowed by the style guide). -- To view, visit http://gerrit.cloudera.org:8080/2266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9b4a04cb8b7b5eb879d017375714b3183be0c601 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
