Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )
Change subject: [clock] introduce time source auto selection ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/15161/8/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/15161/8/src/kudu/clock/hybrid_clock.cc@433 PS8, Line 433: if (clock::BuiltInNtp::auto_configuration_enabled()) { I still don't see the use case for --time_source=auto with --builtin_ntp_enable_auto_config=false. When I asked about this earlier, you said "I don't think --time_source=auto should imply --builtin_ntp_client_enable_auto_config_in_cloud=true: otherwise, there is no way to auto-detect the source and use the custom settings for the --builtin_ntp_servers flag". If a user is going to customize the list of built-in servers, isn't it reasonable to ask them to also set --time_source=builtin? This goes back to my thesis that --time_source=auto and --builtin_ntp_enable_auto_config feel duplicative: I haven't yet been convinced that their separation enables different important use cases. And if we combined them, we could guarantee just one InstanceDetector (in HybridClock), addressing my other concern that it'll be hard to ensure that, if there are multiple InstanceDetectors in the code, only one is active. - Problem: you're running on a cloud environment with --time_source=builtin and you're unhappy with the remote nature of the default NTP servers. Solution: you switch to --time_source=auto. - Problem: you're running on a cloud environment with --time_source=auto, you're unhappy with AWS/GCE NTP and want to further customize your NTP servers. Solution: you switch to --time_source=builtin and modify --builtin_ntp_servers. - Problem: you're running on a cloud environment with --time_source=system and your local machine isn't configured to synchronize well. Solution: you switch to --time_source=auto. Feel free to add/modify the above to prove your argument. http://gerrit.cloudera.org:8080/#/c/15161/8/src/kudu/clock/hybrid_clock.cc@450 PS8, Line 450: back back to -- 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: 8 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 05:49:41 +0000 Gerrit-HasComments: Yes
