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 2019/10/04 06:50:09 UTC

[kudu-CR] WIP [clock] built-in NTP client can auto-detect servers

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


Change subject: WIP [clock] built-in NTP client can auto-detect servers
......................................................................

WIP [clock] built-in NTP client can auto-detect servers

This patch adds the auto-detection mechanism of reference servers
by the built-in NTP client.  The list of the server candidates is
specified by the --builtin_ntp_servers_autodetection_candidates
runtime flag.  By default, AWS and GCE NTP servers (see [1] and [2]) are
put into the list along with public NTP servers from pool.ntp.org.  The
auto-detection is gated by the
--builtin_ntp_servers_autodetection_enabled flag.

WIP:
  * collect some feedback on the approach used
  * add more tests

Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/clock/builtin_ntp.h
M src/kudu/clock/ntp-test.cc
3 files changed, 219 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/14371/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
Gerrit-Change-Number: 14371
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] WIP [clock] built-in NTP client can auto-detect servers

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14371 )

Change subject: WIP [clock] built-in NTP client can auto-detect servers
......................................................................


Patch Set 2:

Interesting approach. I like the reuse of BuiltinNtpClient as a means of detection, since that's the "truest" way of determining whether the source is a real NTP source or not.

That said, I think we may be better off establishing whether we're running on AWS or GCE in a more global sense. You can imagine that information being useful across Kudu, and once we know it definitively, other decisions should become simpler. For example, if we're running on AWS, we should be totally comfortable adding AWS' NTP server to our list of servers unconditionally, without needing to test it at all.

Is that tenable without introducing a startup cost for non-AWS/GCE installations? I'm worried that we'll do detection using e.g. a curl call and be forced to wait for it to timeout after some number of seconds for non-AWS/GCE environments.


-- 
To view, visit http://gerrit.cloudera.org:8080/14371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
Gerrit-Change-Number: 14371
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 04 Oct 2019 21:53:44 +0000
Gerrit-HasComments: No

[kudu-CR] WIP [clock] built-in NTP client can auto-detect servers

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

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

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

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

Change subject: WIP [clock] built-in NTP client can auto-detect servers
......................................................................

WIP [clock] built-in NTP client can auto-detect servers

This patch adds the auto-detection mechanism of reference servers
by the built-in NTP client.  The list of the server candidates is
specified by the --builtin_ntp_servers_autodetection_candidates
runtime flag.  By default, AWS and GCE NTP servers (see [1] and [2]) are
put into the list along with public NTP servers from pool.ntp.org.  The
auto-detection is gated by the
--builtin_ntp_servers_autodetection_enabled flag.

WIP:
  * collect some feedback on the approach used
  * add more tests

[1] https://aws.amazon.com/blogs/aws/keeping-time-with-amazon-time-sync-service/
[2] https://cloud.google.com/compute/docs/instances/managing-instances#configure_ntp_for_your_instances

Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
---
M src/kudu/clock/builtin_ntp.cc
M src/kudu/clock/builtin_ntp.h
M src/kudu/clock/ntp-test.cc
3 files changed, 219 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/14371/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
Gerrit-Change-Number: 14371
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] WIP [clock] built-in NTP client can auto-detect servers

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14371 )

Change subject: WIP [clock] built-in NTP client can auto-detect servers
......................................................................


Patch Set 2:

I like Adar's idea of detecting at a higher level too and then using that to make configuration adjustments. It looks like there are at least a few ways to do this:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/identify_ec2_instances.html


-- 
To view, visit http://gerrit.cloudera.org:8080/14371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie244570cc3b2698c3d4169f3afdc15fdc7ef0364
Gerrit-Change-Number: 14371
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 04 Oct 2019 21:58:53 +0000
Gerrit-HasComments: No