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 (10000 + 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<Subproces > 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 <[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 05:08:03 +0000 Gerrit-HasComments: Yes
