Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )
Change subject: [clock] introduce time source auto selection ...................................................................... Patch Set 6: (1 comment) 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); > This sounds reasonable. I'll set the default value for the glfag then. Th After taking a closer look, I think passing around the auto-detected servers is safer in this case. If controlling the set of servers for the built-in NTP client using the flags, it would be necessary to set default values for the following flags: * --builtin_ntp_servers * --builtin_ntp_client_enable_auto_config_in_cloud As I understand, the idea is to (a) set the default value for --builtin_ntp_servers using the set of the servers found at the time of auto-selecting the time source, and (b) set the default value for --builtin_ntp_client_enable_auto_config_in_cloud to 'false' to avoid doing the same work twice. However, if the value for flag in (b) has been explicitly set to 'true', setting the default value for the flag to 'false' wouldn't help to avoid running the auto-detection of the NTP servers again in BuiltInNtp::InitImpl(). I also thought of getting rid of the --builtin_ntp_client_enable_auto_config_in_cloud flag and using the special value (an empty string) for --builtin_ntp_servers. However, if going that route, it would prevent us from using the custom set of NTP servers to fallback if the auto-configuration fails. So, I think I'll keep the approach of passing the auto-detected NTP servers around. What do you think? -- 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: Wed, 12 Feb 2020 00:05:30 +0000 Gerrit-HasComments: Yes
