Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14317 )
Change subject: [clock] change in the semantics of TimeService::Init() ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/mock_ntp.cc File src/kudu/clock/mock_ntp.cc: http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/mock_ntp.cc@35 PS2, Line 35: if (now_usec) { : *now_usec = mock_clock_time_usec_; : } : if (error_usec) { : *error_usec = mock_clock_max_error_usec_; : } FWIW, I don't see any callers taking advantage of the nullability of now_usec and error_usec, so you could remove those branches. http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/system_ntp.h File src/kudu/clock/system_ntp.h: http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/system_ntp.h@40 PS2, Line 40: // Ensure that the kernel's timekeeping status indicates that it is currently : // in sync, and initialize various internal parameters. Could probably remove this now. http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/system_ntp.cc File src/kudu/clock/system_ntp.cc: http://gerrit.cloudera.org:8080/#/c/14317/2/src/kudu/clock/system_ntp.cc@121 PS2, Line 121: skew_ppm_ = tx.tolerance / kAdjtimexScalingFactor; Does skew_ppm_ need a lock now? Or should it become an atomic type? -- To view, visit http://gerrit.cloudera.org:8080/14317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7245b2cd2c3001a508235f9e539d7202d3ca994 Gerrit-Change-Number: 14317 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 27 Sep 2019 22:32:45 +0000 Gerrit-HasComments: Yes
