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<string> > 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<strings::delimiter::Literal, strings::SkipEmpty>' 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 PS4, Line 597: options.port = 10123 + idx; > If we want this to work well on macOS, we'll need to bind to an ephemral po If I understand correctly, chronyd itself cannot bind to an ephemeral port itself. From the other side, reserving the port somehow and then bind to it requires chronyd using SO_REUSEPORT, but it doesn't. Unfortunately, I cannot see a good solution to make it bullet-proof on macOS. One of the options is to add an extra patch for chronyd, so it would use SO_REUSEPORT and then do the port reservation stuff somehow. 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. Whoops, my bad. Removed. http://gerrit.cloudera.org:8080/#/c/13916/4/src/kudu/mini-cluster/internal_mini_cluster.h@149 PS4, Line 149: if 0 : MiniChronyd* ntp_server(int idx); : #endif removed -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 23 Aug 2019 00:22:35 +0000 Gerrit-HasComments: Yes
