Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7477 )
Change subject: WIP: clock: add a built-in NTP client implementation ...................................................................... Patch Set 9: (14 comments) 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@78 PS9, Line 78: constexpr int kPollIntervalSecs = 30; maybe this should be a flag? http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@255 PS9, Line 255: esrver typo http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@286 PS9, Line 286: time-to-live field : // in the DNS response. I seem to recall I looked into how to find this info and determined that it's actually not possible to do so from any common APIs. That said, if we re-resolve faster than the TTL field in DNS, then I guess we'll end up just hitting a DNS cache and it's no big deal. Wonder how often ntpd or chrony re-resolves http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@385 PS9, Line 385: MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(10); i seem to recall we now have some more general "wait for NTP at startup' thing at a higher layer - maybe this isnt necessary anymore or could be implemented more simply? http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@474 PS9, Line 474: LOG(WARNING) << "invalid NTP packet size from " << from_server.ToString(); maybe we should include the size of the packet, or the packet itself? http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@509 PS9, Line 509: // TODO(todd): why is the server responding with kClient mode? : //resp.protocol_mode() != ntp_packet::kServer) { I guess we should read the RFC carefully and figure this out http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@523 PS9, Line 523: LOG(WARNING) << "root dispersion or delay too large " : << from_server.ToString(); improve this error http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@536 PS9, Line 536: // TODO(todd): handle "kiss-of-death" (RFC 4330 section 8) probably worth doing this http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@583 PS9, Line 583: // TODO(todd) is this right? do we need to add skew*round_trip_delay? should resolve this too http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@595 PS9, Line 595: LOG(INFO) << "NTP from " << from_server->host_.ToString() << " (" probably should be VLOG http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@666 PS9, Line 666: // Run Marzullo's Algorithm with the intervals from all seems like we could probably factor this out and make it testable http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@701 PS9, Line 701: LOG(WARNING) << "false ticker: " << r.addr.ToString(); probably should somehow throttle these http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/builtin_ntp.cc@754 PS9, Line 754: LOG_STRING(ERROR, log) << "TODO"; yea todo http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/ntp-test.cc File src/kudu/clock/ntp-test.cc: http://gerrit.cloudera.org:8080/#/c/7477/9/src/kudu/clock/ntp-test.cc@48 PS9, Line 48: sleep(1); this isn't really a test :) -- 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: 9 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Brock Noland <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[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: Mon, 04 Feb 2019 21:55:28 +0000 Gerrit-HasComments: Yes
