Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15156 )
Change subject: [clock] simplify system time source ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/15156/3/src/kudu/clock/system_ntp.cc File src/kudu/clock/system_ntp.cc: http://gerrit.cloudera.org:8080/#/c/15156/3/src/kudu/clock/system_ntp.cc@74 PS3, Line 74: Both CallAdjTime and NtpGetTime are only used once, so I think it'd be more clear to inline them in their call sites rather than pull them out into these tiny helpers. http://gerrit.cloudera.org:8080/#/c/15156/3/src/kudu/clock/system_ntp.cc@81 PS3, Line 81: Status NtpGetTime(ntptimeval* tv) { Can you rework this a bit to avoid the call to ntp_gettime() if FLAGS_inject_unsyc_time_errors is true? http://gerrit.cloudera.org:8080/#/c/15156/3/src/kudu/clock/system_ntp.cc@82 PS3, Line 82: int rc = ntp_gettime(tv); Regarding return values, the manpage says: The return values for ntp_gettime() and ntp_gettimex() are as for adjtimex(2). Given a correct pointer argument, these functions always succeed. Given that we can guarantee 'tv' is writable, perhaps we should PCHECK the result? http://gerrit.cloudera.org:8080/#/c/15156/3/src/kudu/clock/system_ntp.cc@133 PS3, Line 133: tv.time.tv_sec * 1000000 Can this overflow? Do we need to explicitly cast one operand to uint64_t? -- To view, visit http://gerrit.cloudera.org:8080/15156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib197effb63e8f5e37fd828c88e705112bb7047ce Gerrit-Change-Number: 15156 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Tue, 04 Feb 2020 21:29:35 +0000 Gerrit-HasComments: Yes
