Adar Dembo 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 107: Timestamp now = clock->Now(); Maybe I'm misunderstanding how this works, but couldn't we assert more strongly, that when 1000 < time < 1013, Now() == 1012, and when 1013 <= time < 1020, Now() == time? http://gerrit.cloudera.org:8080/#/c/2266/3/src/kudu/server/hybrid_clock.cc File src/kudu/server/hybrid_clock.cc: Line 234: uint64_t candidate_phys_timestamp = now_usec << kBitsToShift; Why not build a Timestamp immediately and use CompareTo() below? Line 265: *max_error_usec = (next_timestamp_ >> kBitsToShift) - (now_usec - error_usec); Then you could also use GetPhysicalValueMicros() here to extract the left hand side of the subtraction. http://gerrit.cloudera.org:8080/#/c/2266/3/src/kudu/server/hybrid_clock.h File src/kudu/server/hybrid_clock.h: Line 193: uint64_t next_timestamp_; My other comments kind of touched on this: it would be nice if this could be a Timestamp type and encapsulate as much of the arithmetic as possible in Timestamp and HybridClock methods. I might be missing something about why we can't do that though; this is new code to me. -- 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-HasComments: Yes
