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<string> 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 22 Aug 2019 18:49:07 +0000 Gerrit-HasComments: Yes
