Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7477 )
Change subject: [clock] add a built-in NTP client implementation ...................................................................... Patch Set 17: (9 comments) http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h File src/kudu/clock/builtin_ntp.h: http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h@44 PS17, Line 44: // It's not RFC-compliant yet. Worth summarizing the big gaps? Or is that too much detail? http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h@48 PS17, Line 48: rely relies http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h@49 PS17, Line 49: estimated estimate http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc File src/kudu/clock/builtin_ntp.cc: http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@385 PS9, Line 385: void InvalidatePacket() { > This has been addressed in one of the recent patch series. The idea is to I was hoping for a consolidation. In pseudocode, it's: "check if time is synchronized every second or so until it is, or until a user-configurable timeout expires". That should be implementable once without regard to the underlying time service being used. http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc File src/kudu/clock/builtin_ntp.cc: http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc@352 PS13, Line 352: : } > Well, it seems to be implemented already: see line 732 in PS13. Yeah there was a call to NextServer there, but no reresolution. The new code works like this too (i.e. no calls to reresolve). But you mentioned that this is no longer a best practice in a different comment. http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc@572 PS13, Line 572: } > Yes, but what if there is more data than needed in the socket? I'm not sur Maybe I'm misunderstanding, but couldn't you just recvfrom() one NtpPacket's worth of data, then recvfrom() the rest (one NtpPacket at a time) in subsequent poll loop invocations? http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc@717 PS13, Line 717: n - sizeof(NtpPacket), n, from_server.ToString()); > Done. Why pass by cref and not move it? http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.cc File src/kudu/clock/builtin_ntp.cc: http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.cc@73 PS17, Line 73: // RFC 4330 mandates in the 'Best practices' lists 15 seconds as the minimum : // allowed interval, but RFC 5905 scapped the whole 'Best practices' section. I might move this to after L79 and prefix it with "Note:" because it's basically just a curiosity. Also, "scrapped". http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.cc@427 PS17, Line 427: signle single -- To view, visit http://gerrit.cloudera.org:8080/7477 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb9eee9f0334236f39617492bd6f01304d1a0255 Gerrit-Change-Number: 7477 Gerrit-PatchSet: 17 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 24 Sep 2019 01:27:24 +0000 Gerrit-HasComments: Yes
