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: (2 comments) 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); > I don't like the idea of passing parameters via global variables. That's w Agreed with the global variable sentiment, but worth pointing out that there's a difference between changing the _value_ of a gflag, and changing its _default value_. I think the latter is perfectly reasonable as it still allows the user to provide their own value and override whatever we've chosen when we changed the default value. To summarize, the priority order is: 1. user-provided value, which takes precedence over 2. dynamic default value (chosen at runtime), which takes precedence over 3. static default value (baked into the code). 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; > Apart from this particular piece, I thought about introducing the --time_so I guess what you want is to be able to tag a particular gflag _value_ as unsafe (rather than tagging the entire gflag). And then you'd tag "system_unsync' in this way, forcing people to use --unlock_unsafe_flags. Maybe you could get that same effect using a group vaiidator for --time_source, without introducing a brand new gflag? -- 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 22:46:59 +0000 Gerrit-HasComments: Yes
