Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )
Change subject: [clock] introduce time source auto selection ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/15161/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15161/1//COMMIT_MSG@10 PS1, Line 10: haves a behavior 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: wait_for_usec, us_until_deadline)); > I changed the default for --builtin_ntp_client_enable_auto_config_in_cloud I'm still finding it really difficult to reason about when the HybridClock cloud detector runs, and when the BuiltInNtp detector runs, and whether they ever both run. Moreover, I'm finding it hard to figure out exactly what all the knobs are, which ones the user is likely to use, which ones they aren't, and whether we've achieved the right level of flexibility here (or whether we've overengineered). Could you add a summary description of the gflags and their interactions to the commit message? You can ignore the ones that affect some small aspect of a time source (like --builtin_ntp_true_time_refresh_max_interval_s) and focus on just the ones that affect time source selection and, for the built-in NTP client, NTP server selection. 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@416 PS6, Line 416: result_builtin_ntp_servers.emplace_back(ntp_server, : clock::kStandardNtpPort); An alternative to passing the builtin servers back and forth is, in this case (i.e. when we've detected the cloud provider and we know we should use an alternate NTP server), to use the gflag API to change the default value of --builtin_ntp_servers to whatever we want it to be. That should reduce the amount of work in the code that follows. http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@428 PS6, Line 428: if (result_time_source == TimeSource::UNKNOWN) { : // The fallback to TimeSource::SYSTEM_UNSYNC is possible only when : // it's explicitly allowed by --time_source_system_unsync_allowed flag. : if (!FLAGS_time_source_system_unsync_allowed) { : return Status::InvalidArgument( : "no viable time source to auto-select: in a test environment " : "consider setting --time_source_system_unsync_allowed"); : } : result_time_source = TimeSource::SYSTEM_UNSYNC; In practice, we only wind up here on older versions of macOS, right? I don't think we need --time_source_system_unsync_allowed for them; I think we can drop the gflag and just choose SYSTEM_UNSYNC unconditionally for such cases. Let's just make sure we LOG the chosen time source in the 'auto' case. -- 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: 6 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 20:44:11 +0000 Gerrit-HasComments: Yes
