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: (1 comment) 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()) { > Ah, and the importance of the '--time_source=builtin --builtin_ntp_enable_a Regarding instances of InstanceDetector: agreed that the current code tries to ensure that only one is ever used. But, if there's a bug, or if it regresses in the future, we'll slow down startup in a way that's not necessary obvious. I'd rather there be just one InstanceDetector instance across the entire server, and that instance "move up" the module tree as needed. Meaning, it can start in HybridClock because that's the only place it's needed now, and if we need it in some other part of the server in the future, it 'll move out of HybridClock and into ServerBase. This is the safest way to guarantee that we'll never ever duplicate instance detection, at a cost of some extra plumbing. Regarding --time_source=builtin and --builtin_ntp_enable_auto_config=true, agreed that it is a compelling configuration for "use the built-in NTP client no matter what, with the best possible server list for each environment." The thing is, the value of "use the built-in NTP client no matter what" is somewhat eroded after considering Azure, where the "best" choice is the system time source. Is preserving that use case worth the added complexity of the additional --builtin_ntp_enable_auto_config knob, which every AWS/GCE user will want to use? I don't disagree that there's some uncertainty here, but I think we're better off starting with fewer knobs and adding more in response to feedback rather than the other way around. -- 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 08:22:44 +0000 Gerrit-HasComments: Yes
