Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7477 )
Change subject: [clock] add a built-in NTP client implementation ...................................................................... Patch Set 18: (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: // * support of iburst/burst operation modes (see KUDU-2937) > Worth summarizing the big gaps? Or is that too much detail? Done http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h@48 PS17, Line 48: 2941 > relies Done http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.h@49 PS17, Line 49: > estimate Done 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: ++i_pkt_total_num_; > I was hoping for a consolidation. In pseudocode, it's: "check if time is sy Done 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: : } > Yeah there was a call to NextServer there, but no reresolution. The new cod Ah, indeed: no re-resolution is done now. We can implement that optimization using DnsResolver::ResolveAddressesAsync(). I think it's better to do so in a separate changelist; I'll add TODO for a while. http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc@572 PS13, Line 572: } > Maybe I'm misunderstanding, but couldn't you just recvfrom() one NtpPacket' My concern is the following: if the packet that arrived has more data than we expect and we call recvfrom() only for a part of it, then 1) we don't know whether the actual packet was larger 2) next call of recvfrom() will wait up for the timeout, eventually retrieving the rest of the data. In that case we wouldn't know that the actual packet was larger if the data we read at step 1 was looking good to us, and that way we will add extra delay at step 2. Maybe, we should read the data with MSG_PEEK flag first, and then proceed with reading it into NtpPacket or discard if the amount of data is greater than we expect? http://gerrit.cloudera.org:8080/#/c/7477/13/src/kudu/clock/builtin_ntp.cc@717 PS13, Line 717: // (unicast) or 5 (broadcast). > Why pass by cref and not move it? As it turned out, I was mistaken: RecordedResponse is trivially-copyable regardless of SockAddr field (which is trivially copyable itself). src/kudu/clock/builtin_ntp.cc:844:29: warning: std::move of the variable 'rr' of the trivially-copyable type 'kudu::clock::BuiltInNtp::RecordedResponse' has no effect; remove std::move() [performance-move-const-arg] RecordResponse(p->server, std::move(rr)); 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: // In the 'Best practices' section, RFC 4330 states that 15 seconds is the : // minimum allowed polling interval. > I might move this to after L79 and prefix it with "Note:" because it's basi Done http://gerrit.cloudera.org:8080/#/c/7477/17/src/kudu/clock/builtin_ntp.cc@427 PS17, Line 427: the s > single Done -- 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: 18 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: Wed, 25 Sep 2019 22:15:19 +0000 Gerrit-HasComments: Yes
