[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13916 ) Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. In addition, this patch contains tests to cover the newly introduced functionality. Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Reviewed-on: http://gerrit.cloudera.org:8080/13916 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M build-support/dist_test.py M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h 8 files changed, 802 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13916 ) Change subject: [clock] introduce mini_chronyd .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 26 Aug 2019 22:10:11 + Gerrit-HasComments: No
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13916 )
Change subject: [clock] introduce mini_chronyd
..
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc
File src/kudu/clock/test/mini_chronyd-test.cc:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc@143
PS4, Line 143: ASSERT_OK(servers[i]->SetTime(ref_time + 10));
> The new comment is clear but I don't see how the code follows it: the argum
Whoops, good catch! That's was a typo.
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@73
PS5, Line 73:
> "the offset"?
Done
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@75
PS5, Line 75: "se
> has
Done
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@75
PS5, Line 75: burst version 3\n";
: if (servers.empty()) {
> "deem them to be a reliable NTP clock source first"?
Done
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@76
PS5, Line 76:
> successful
Done
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@78
PS5, Line 78:
> "appear to be a"
Done
http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@82
PS5, Line 82: }
> Mind reformatting this as a list, so it's easier to add/remove configuratio
Done
--
To view, visit http://gerrit.cloudera.org:8080/13916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7
Gerrit-Change-Number: 13916
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 26 Aug 2019 14:49:15 +
Gerrit-HasComments: Yes
[kudu-CR] [clock] introduce mini chronyd
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13916 to look at the new patch set (#6). Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. In addition, this patch contains tests to cover the newly introduced functionality. Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 --- M build-support/dist_test.py M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h 8 files changed, 802 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/13916/6 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13916 ) Change subject: [clock] introduce mini_chronyd .. Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc File src/kudu/clock/test/mini_chronyd-test.cc: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc@143 PS4, Line 143: // servers as a reliable source for time synchronisation via NTP. > It doesn't matter from which point they are all offset, the important thing The new comment is clear but I don't see how the code follows it: the argument value passed to SetTime() is the same for all five servers. http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc File src/kudu/clock/test/mini_chronyd.cc: http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@73 PS5, Line 73: offset "the offset"? http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@75 PS5, Line 75: have has http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@76 PS5, Line 76: successfully successful http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@75 PS5, Line 75: find them as a reliable : // NTP clock source first "deem them to be a reliable NTP clock source first"? http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@78 PS5, Line 78: seem as "appear to be a" http://gerrit.cloudera.org:8080/#/c/13916/5/src/kudu/clock/test/mini_chronyd.cc@82 PS5, Line 82: // The 'minpoll' parameter is set to the smallest possible that is supported Mind reformatting this as a list, so it's easier to add/remove configuration elements later? -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 26 Aug 2019 04:46:29 + Gerrit-HasComments: Yes
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13916 ) Change subject: [clock] introduce mini_chronyd .. Patch Set 5: Verified+1 Unrelated flake due to the issue reported in KUDU-2815 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 23 Aug 2019 02:23:32 + Gerrit-HasComments: No
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has removed a vote on this change. Change subject: [clock] introduce mini_chronyd .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13916 to look at the new patch set (#5). Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. In addition, this patch contains tests to cover the newly introduced functionality. Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 --- M build-support/dist_test.py M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h 8 files changed, 798 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/13916/5 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13916 )
Change subject: [clock] introduce mini_chronyd
..
Patch Set 4:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/CMakeLists.txt
File src/kudu/clock/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/CMakeLists.txt@39
PS4, Line 39: # * symlinks would not work with dist-test
> But don't we use symlinks for the HMS/Sentry/Hadoop stuff? Why do they work
Yes, we do, but those are symlinks to directories, not regular files. I didn't
look deep into the internals of dist-test, but I know dist-test copies contents
of symlinked directories, but it doesn't follow symlinks in case of regular
files.
I tried symlinks for chronyd and chronyc first, but that didn't work. That's
why I switched to simply copying those files.
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc
File src/kudu/clock/test/mini_chronyd-test.cc:
PS4:
> Have you looped the new tests under dist-tes?
Yes, I did it some time ago and re-run again today:
http://dist-test.cloudera.org//job?job_id=aserbin.1566515381.147044
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc@143
PS4, Line 143: ASSERT_OK(servers[i]->SetTime(ref_time + 10));
> Was the intent to reconfigure each server to use the same time albeit one f
It doesn't matter from which point they are all offset, the important thing
here is that they are far from each other. Eventually, we want to make sure
the client does _not_ see these NTP servers as a reliable source of
synchronization.
In prior revisions of this patch, I did set the same time and it worked most of
the time because the it took some time per operation, and if everything goes
slow enough, servers seem far enough to the client. To make this more robust
and explicit, I changed this to use different, far enough from each other
points in time.
I updated the comment, hopefully it became clearer now.
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h
File src/kudu/clock/test/mini_chronyd.h:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@87
PS4, Line 87: // Structure to represent relevant information from output by
: // 'chronyc serverstats'.
> Nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@94
PS4, Line 94: as good
> as a good
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@99
PS4, Line 99: synchronize
> synchronization
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@105
PS4, Line 105: of
> or
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@72
PS4, Line 72: std::
> drop
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@75
PS4, Line 75: "server $0 port $1 maxpoll -1 minpoll -6 iburst burst
version 3\n";
> Could you rationalize the non-obvious stuff (i.e. everything besides server
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@88
PS4, Line 88: /dev/stdin
> Does "-" work? I think it's more idiomatic.
Yeah, I tried '-' first. Unfortunately, it doesn't work for chronyd. It
possible to put together an extra patch for chronyd, but I decided not to spend
time on that.
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@196
PS4, Line 196: vector
> Can you use 'auto' here?
It does't compile:
kudu/src/kudu/clock/test/mini_chronyd.cc:216:14: error: no member named
'size' in 'strings::internal::Splitter'
if (kv.size() != 2) {
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@204
PS4, Line 204: LOG(INFO) << kv[0] << ":" << kv[1];
> Should be removed?
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@206
PS4, Line 206: result.ntp_packets_received = std::atol(kv[1].c_str());
> I think we prefer our gutil/strings/numbers.h safe_* functions.
Done
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.h@195
PS4, Line 195: int num_ntp_servers;
> Should add test coverage for this in external_mini_cluster-test.
Yeap, this is coming in the follow-up changelist.
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.cc@597
[kudu-CR] [clock] introduce mini chronyd
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13916 ) Change subject: [clock] introduce mini_chronyd .. Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/CMakeLists.txt File src/kudu/clock/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/CMakeLists.txt@39 PS4, Line 39: # * symlinks would not work with dist-test But don't we use symlinks for the HMS/Sentry/Hadoop stuff? Why do they work there but not here? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc File src/kudu/clock/test/mini_chronyd-test.cc: PS4: Have you looped the new tests under dist-tes? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd-test.cc@143 PS4, Line 143: ASSERT_OK(servers[i]->SetTime(ref_time + 10)); Was the intent to reconfigure each server to use the same time albeit one far removed from NOW? Or for each server to use a different time? Unclear from the comment. http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h File src/kudu/clock/test/mini_chronyd.h: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@87 PS4, Line 87: // Structure to represent relevant information from output by : // 'chronyc serverstats'. Nit: alignment http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@94 PS4, Line 94: as good as a good http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@99 PS4, Line 99: synchronize synchronization http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.h@105 PS4, Line 105: of or http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc File src/kudu/clock/test/mini_chronyd.cc: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@72 PS4, Line 72: std:: drop http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@75 PS4, Line 75: "server $0 port $1 maxpoll -1 minpoll -6 iburst burst version 3\n"; Could you rationalize the non-obvious stuff (i.e. everything besides server and port) in a comment? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@88 PS4, Line 88: /dev/stdin Does "-" work? I think it's more idiomatic. http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@196 PS4, Line 196: vector Can you use 'auto' here? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@204 PS4, Line 204: LOG(INFO) << kv[0] << ":" << kv[1]; Should be removed? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/clock/test/mini_chronyd.cc@206 PS4, Line 206: result.ntp_packets_received = std::atol(kv[1].c_str()); I think we prefer our gutil/strings/numbers.h safe_* functions. http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.h@195 PS4, Line 195: int num_ntp_servers; Should add test coverage for this in external_mini_cluster-test. http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/external_mini_cluster.cc@597 PS4, Line 597: options.port = 10123 + idx; If we want this to work well on macOS, we'll need to bind to an ephemral port and then use lsof or whatever to figure out what port we got. Can we do that? http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/internal_mini_cluster.h File src/kudu/mini-cluster/internal_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/internal_mini_cluster.h@111 PS4, Line 111: #if 0 Why commented out? Either remove or implement. -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 22 Aug 2019 18:49:07 + Gerrit-HasComments: Yes
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13916 )
Change subject: [clock] introduce mini_chronyd
..
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h
File src/kudu/clock/test/mini_chronyd.h:
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@40
PS3, Line 40: //
> Nit: add an empty line between this and the previous line.
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@53
PS3, Line 53: //
> What effect will this have on the running chronyd?
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@67
PS3, Line 67: // Default: 10123 (1 + the default NTP port).
> Should doc the defaults of the below options, and what effects the defaults
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@85
PS3, Line 85:
> Nit: drop 'the'
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@90
PS3, Line 90: int64_t cmd_packets_received;
: int64_t ntp_packets_received;
: };
:
: // Check that NTP servers with the specified end
> Agreed with all of this. Follow-on work?
This is done in a follow-up changelist. Basically, the code in
ExternalMiniCluster takes care of that.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@102
PS3, Line 102:int timeout_sec = 3)
: WARN_UNUSE
> Maybe combine with the one-arg ctor?
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@95
PS3, Line 95: Substitute("failed measure clock offset from reference NTP
servers: "
: "stdout{$0} stderr{$1}",
: out_stdout, out_stderr));
: r
> This isn't typically defined. Seems like you should follow what MiniSentry/
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@135
PS3, Line 135: RETURN_NOT_OK(Env::Default()->CreateDir(options_.data_root));
: // The chronyd's implementation puts strict requirements on
the ownership
: // of the directories where the runtime data is stor
> Yeah but would we actually expect data_root to have been created by someone
That's about group ownership of the directory. On my laptop, the created
directory is owned by group wheel (I guess since my account has local admin
role), but GID of my account is different.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@151
PS3, Line 151:
> The username needs to be specified on the command line and in the config fi
Good catch. Nope, it's not needed in the command line once it's in the config
file. However, it's possible to override the value specified in the config
file by command-line flag.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@180
PS3, Line 180: VLOG(1) << "stopping chronyd";
: unique_ptr Want to use KillAndWait() here?
Done
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@195
PS3, Line 195: for (StringPiece sp : Split(out, "\n", SkipEmpty())) {
> Probably merits more context.
Ah, that was just for debug. I removed the LOG() message.
http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@220
PS3, Line 220:
: Status MiniChronyd::SetTime(time_t time) {
: char buf[kFastToBufferSize];
: char* time_to_set = FastTimeToBuffer(time, buf);
: return RunChronyCmd({ "settime", time_to_set });
: }
:
> Would also be nice to doc what these mean, so the config file is more reada
Done
--
To view, visit http://gerrit.cloudera.org:8080/13916
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7
Gerrit-Change-Number: 13916
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 22 Aug 2019 05:08:03 +
Gerrit-HasComments: Yes
[kudu-CR] [clock] introduce mini chronyd
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13916 to look at the new patch set (#4). Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. In addition, this patch contains tests to cover the newly introduced functionality. Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 --- M build-support/dist_test.py M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.h 9 files changed, 777 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/13916/4 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13916 to look at the new patch set (#2). Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. WIP: * add more tests * add more useful functionality to use for follow-up tests running built-in NTP client Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 --- M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h 4 files changed, 537 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/13916/2 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [clock] introduce mini chronyd
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13916 Change subject: [clock] introduce mini_chronyd .. [clock] introduce mini_chronyd This patch introduces a wrapper around chronyd, so it's possible to run multiple instances of chronyd reference NTP servers as part of Kudu mini-cluster, providing reference for NTP clients. One nice feature that chronyd has is its ability to run in server-only mode, i.e. not driving the system clock: see '-x' option 'man chronyd'. Also, it's possible to run using the system clock as a source instead of a physical device (GPS, oscillator, etc.). In addition, it's possible to manually set the reference time for chrony NTP server running in server-only local mode. The chrony suite has 'chronyc' CLI tool to send control commands to chronyd NTP daemon. WIP: * add more tests * add more useful functionality to use for follow-up tests running built-in NTP client Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 --- M src/kudu/clock/CMakeLists.txt A src/kudu/clock/test/mini_chronyd-test.cc A src/kudu/clock/test/mini_chronyd.cc A src/kudu/clock/test/mini_chronyd.h 4 files changed, 537 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/13916/1 -- To view, visit http://gerrit.cloudera.org:8080/13916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id9d06d218828240f2a2980ef5ec30428f86277f7 Gerrit-Change-Number: 13916 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
