Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )
Change subject: [clock] introduce time source auto selection ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/15161/1/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/15161/1/src/kudu/clock/hybrid_clock.cc@175 PS1, Line 175: const auto s = detector.Detect(&md); > I'm still finding it really difficult to reason about when the HybridClock Sure, I'll add that into the commit message. As a summary, for the auto-selection of the time source and the auto-detection of the NTP server for the built-in NTP client there are two orthogonal flags: * --time_source=auto for the former * --builtin_ntp_client_enable_auto_config_in_cloud=true for the latter http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@91 PS6, Line 91: } > Looks a bit cryptic. I suggest to replace macro with text (here and below). The idea was to avoid using string literals, but use constants (helps with getting away from typo-style mistakes). http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@416 PS6, Line 416: if (now > then) { : return Status::OK(); > An alternative to passing the builtin servers back and forth is, in this ca I don't like the idea of passing parameters via global variables. That's why I decided to go this route.] Also, I'd like to keep the values of those flags as they are set by user (that's for reporting on chosen parameters, etc.). http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@428 PS6, Line 428: : bool HybridClock::IsAfter(Timestamp t) { : // Manually get the time, rather than using Now(), so we don't end up causing : // a time update. : uint64_t now_usec; : uint64_t error_usec; : WalltimeWithErrorOrDie(&now_usec, &error_usec); : : Timestamp now; > In practice, we only wind up here on older versions of macOS, right? I don' Apart from this particular piece, I thought about introducing the --time_source_system_unsync_allowed flag anyways: I think there should be explicit guardrail to prevent people using the 'system_unsync' time source in production. If necessary, I can move the introduction of the --time_source_system_unsync_allowed flag into a separate changelist. What do you think? http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@466 PS6, Line 466: // A B C > Should it be wrapped into #if defined(KUDU_HAS_SYSTEM_TIME_SOURCE) ? I think there is no need for that. This is just an utility to convert enum values into strings, it shouldn't be affected by the availability of the 'system' time source. -- To view, visit http://gerrit.cloudera.org:8080/15161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4 Gerrit-Change-Number: 15161 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Fri, 07 Feb 2020 22:26:19 +0000 Gerrit-HasComments: Yes
