Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13916 )
Change subject: WIP [clock] introduce mini_chronyd ...................................................................... Patch Set 3: (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: // Default: 0 Nit: add an empty line between this and the previous line. http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@53 PS3, Line 53: // Default: "" What effect will this have on the running chronyd? http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@67 PS3, Line 67: Should doc the defaults of the below options, and what effects the defaults have. http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@85 PS3, Line 85: the Nit: drop 'the' http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@90 PS3, Line 90: // TODO(aserbin): how to run multiple chronyd processes for the same minicluster : // Introduce an index, and in case if not using UNIQUE_LOOPBACK : // it's also necessary to dedicated ports. : // In the latter case, resrve the ports the same way as it's done : // for multi-master configurations. Agreed with all of this. Follow-on work? http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.h@102 PS3, Line 102: // Creates a new MiniChronyd with the default options. : MiniChronyd(); Maybe combine with the one-arg ctor? explicit MiniChronyd(MiniChronydOptions options = MiniChronydOptions()); 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: if (!kudu_home) { : return Status::ConfigurationError( : "KUDU_HOME environment variable is not defined"); : } This isn't typically defined. Seems like you should follow what MiniSentry/MiniHMS do. Same below. http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@135 PS3, Line 135: // The chronyd's implementation puts strict requirements on the ownership : // of the directories where the runtime data is stored. : RETURN_NOT_OK(CorrectOwnership(options_.data_root)); Yeah but would we actually expect data_root to have been created by someone else? If the desired usage is tests, wouldn't the same user be responsible for all of this? http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@151 PS3, Line 151: "-u", username, The username needs to be specified on the command line and in the config file? http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@180 PS3, Line 180: RETURN_NOT_OK(proc->Kill(SIGTERM)); : return proc->Wait(); Want to use KillAndWait() here? http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@195 PS3, Line 195: LOG(INFO) << "SETTIME " << time_to_set; Probably merits more context. http://gerrit.cloudera.org:8080/#/c/13916/3/src/kudu/clock/test/mini_chronyd.cc@220 PS3, Line 220: user $0 : bindaddress $1 : bindcmdaddress $2 : driftfile $3 : port $4 : dumpdir $5 : pidfile $6 Would also be nice to doc what these mean, so the config file is more readable. -- 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: 3 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2019 19:31:11 +0000 Gerrit-HasComments: Yes
