You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/02/05 02:15:37 UTC

[kudu-CR] [clock] auto-selection of time source

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15161


Change subject: [clock] auto-selection of time source
......................................................................

[clock] auto-selection of time source

This patch introduces a new time source named 'auto'.  When
--time_source flag is set to 'auto', the system's behaves is the
following:

  * If the host is of known cloud instance type that provides
    a dedicated NTP server, the time source is set to 'builtin'.

  * If the host is of known cloud instance type that doesn't provide
    a dedicated NTP server or the host is not recognized as a cloud
    instance, the time source is set to 'system'
    (if 'system' time source is supported, otherwise set to
     'system_unsync').

In former case, if the --builtin_ntp_client_enable_auto_config_in_cloud
flag is set to 'true', the built-in NTP client is configured to use
the dedicated NTP server provided by the cloud platform.

Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
---
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/util/cloud/instance_metadata.cc
M src/kudu/util/cloud/instance_metadata.h
4 files changed, 63 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/15161/1
-- 
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: newchange
Gerrit-Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
Gerrit-Change-Number: 15161
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [clock] introduce time source auto selection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, Volodymyr Verovkin, Hao Hao, Bankim Bhavsar, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15161

to look at the new patch set (#10).

Change subject: [clock] introduce time source auto selection
......................................................................

[clock] introduce time source auto selection

This patch introduces a new pseudo time source named 'auto'.  When
the --time_source flag is set to 'auto', Kudu masters and tablet
servers automatically select the most appropriate time source for
the hybrid clock (see below for the rationale behind the choice).

Below are the rules for the time source auto-selection:

  * AWS and GCE cloud environments: the 'builtin' time source is
    used (i.e. the hybrid clock uses the built-in NTP client) and
    the built-in NTP client is configured to use the dedicated NTP
    server provided by the environment

  * Azure cloud environments, not-yet-supported cloud environments,
    and non-cloud machines: the 'system' time source is used
    (i.e. the hybrid clock uses NTP-synchronized local clock)

  * macOS machines:
      ** High Sierra 10.13 and newer: 'system'
      ** releases prior to High Sierra: 'system_unsync'

As for the rationale behind auto-selecting this or another time source:

  * when NTP servers are not known beforehand or their high availability
    is not guaranteed, relying on the local NTP-synchronized clock is
    the only rational choice (and it's backward-compatible)

  * when it's guaranteed to have a set of highly available NTP servers,
    using the built-in NTP client is the best choice because it removes
    the requirement to configure and maintain NTP server at the machine

NOTE: this patch essentially removes the functionality introduced in
      69cf2065805163799d85dc38b812eb73388d3a02 (i.e. auto-configuration
      of the built-in NTP client), so now automatic configuration of the
      built-in NTP client is not available outside of the
      --time_source=auto context.

Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/clock/builtin_ntp.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 274 insertions(+), 126 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/15161/10
-- 
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: newpatchset
Gerrit-Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
Gerrit-Change-Number: 15161
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [clock] introduce time source auto selection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 22:46:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] [clock] introduce time source auto selection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

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:

(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);
> Agreed with the global variable sentiment, but worth pointing out that ther
This sounds reasonable.  I'll set the default value for the glfag then.  Thanks.


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;
> I guess what you want is to be able to tag a particular gflag _value_ as un
Indeed, the need is to mark a particular gflag value as unsafe.

I think using the group validator for that purpose is a great idea.  Thanks!



-- 
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 23:11:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] [clock] introduce time source auto selection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Feb 2020 05:49:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] [clock] introduce time source auto selection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )

Change subject: [clock] introduce time source auto selection
......................................................................


Patch Set 4:

unrelated test failure:
  * RELEASE: https://issues.apache.org/jira/browse/KUDU-3051


-- 
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: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 05:51:48 +0000
Gerrit-HasComments: No

[kudu-CR] [clock] introduce time source auto selection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15161 )

Change subject: [clock] introduce time source auto selection
......................................................................


Patch Set 1:

(5 comments)

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:     const auto s = detector.Detect(&md);
> I'm still finding it really difficult to reason about when the HybridClock 
Sure, I'll add that into the commit message.

As a summary, for the auto-selection of the time source and the auto-detection of the NTP server for the built-in NTP client there are two orthogonal flags:
  * --time_source=auto for the former
  * --builtin_ntp_client_enable_auto_config_in_cloud=true for the latter


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@91
PS6, Line 91:   }
> Looks a bit cryptic. I suggest to replace macro with text (here and below).
The idea was to avoid using string literals, but use constants (helps with getting away from typo-style mistakes).


http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@416
PS6, Line 416:   if (now > then) {
             :     return Status::OK();
> An alternative to passing the builtin servers back and forth is, in this ca
I don't like the idea of passing parameters via global variables.  That's why I decided to go this route.]

Also, I'd like to keep the values of those flags as they are set by user (that's for reporting on chosen parameters, etc.).


http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@428
PS6, Line 428: 
             : bool HybridClock::IsAfter(Timestamp t) {
             :   // Manually get the time, rather than using Now(), so we don't end up causing
             :   // a time update.
             :   uint64_t now_usec;
             :   uint64_t error_usec;
             :   WalltimeWithErrorOrDie(&now_usec, &error_usec);
             : 
             :   Timestamp now;
> In practice, we only wind up here on older versions of macOS, right? I don'
Apart from this particular piece, I thought about introducing the --time_source_system_unsync_allowed flag anyways: I think there should be explicit guardrail to prevent people using the 'system_unsync' time source in production.

If necessary, I can move the introduction of the --time_source_system_unsync_allowed flag into a separate changelist.

What do you think?


http://gerrit.cloudera.org:8080/#/c/15161/6/src/kudu/clock/hybrid_clock.cc@466
PS6, Line 466:     //  A         B          C
> Should it be wrapped into #if defined(KUDU_HAS_SYSTEM_TIME_SOURCE) ?
I think there is no need for that.  This is just an utility to convert enum values into strings, it shouldn't be affected by the availability of the 'system' time source.



-- 
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: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 22:26:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] [clock] introduce time source auto selection

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, Volodymyr Verovkin, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15161

to look at the new patch set (#5).

Change subject: [clock] introduce time source auto selection
......................................................................

[clock] introduce time source auto selection

This patch introduces a new time source named 'auto'.  When the
--time_source flag is set to 'auto', the system behaves as the
following:

  * If the host is of known cloud instance type that provides
    a dedicated NTP server, the time source is set to 'builtin'.

  * If the host is of known cloud instance type that doesn't provide
    a dedicated NTP server or the host is not recognized as a cloud
    instance, the time source is set to 'system'.

In the former case, since --builtin_ntp_client_enable_auto_config_in_cloud
is set to 'true' by default, the built-in NTP client is configured
to use the dedicated NTP server provided by the cloud platform.  To
disable the auto-configuration of the built-in NTP client and use
custom list of NTP servers specified by the --builtin_ntp_servers flag,
set --builtin_ntp_client_enable_auto_config_in_cloud=false.

Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
---
M src/kudu/client/client_examples-test.sh
M src/kudu/clock/builtin_ntp.cc
M src/kudu/clock/builtin_ntp.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/util/test_util.cc
8 files changed, 289 insertions(+), 82 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/15161/5
-- 
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: newpatchset
Gerrit-Change-Id: If9f2bc94f4a1ad32fe365d4f6f0707913fb476a4
Gerrit-Change-Number: 15161
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [clock] introduce time source auto selection

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 20:44:11 +0000
Gerrit-HasComments: Yes