Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14227 )
Change subject: [mini_cluster] introduce 'builtin' clock source ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/14227/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14227/2//COMMIT_MSG@13 PS2, Line 13: server-only > Can you explain a bit on this mode and why do we use it? Done. I added a small note on what server-only mode means. Also, you can see one of the prior changes for more details on how chronyd is used in our tests scaffolding: https://github.com/apache/kudu/commit/0f38821c4867b86044be22dbceb80c7bc04b5d33 http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/integration-tests/master_migration-itest.cc File src/kudu/integration-tests/master_migration-itest.cc: http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/integration-tests/master_migration-itest.cc@202 PS2, Line 202: // unique_ptr wrapper by calling 'std::unique_ptr::reset()'. > What is the reason for this change? The reason for this change is that upon call of ExternalMinicluster::Shutdown() only Kudu masters and tablet servers are shut down, all the rest of helper processes in minicluster (such as kdc, chronyd, HMS, Sentry) are not still running. The migrated cluster uses the same parameters as the former one, so another instance of chronyd is about to start at the same port, and it fails. http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/mini-cluster/external_mini_cluster.h@190 PS2, Line 190: // Number of NTP servers to start as part of the cluster. If 0 or less, : // no NTP servers are started. The NTP servers are used as time references : // for the NTP client built into masters and tablet servers. : // : // Default: 0 : int num_ntp_servers; : }; : : // A mini-cluster made up of subprocesses running each of the daemons : // separately. This is useful for black-box or grey-box failure testing : // purposes -- it provides the ability to forcibly kill or stop particular : // cluster participants, which isn't feasible in the normal InternalMiniCluster. : // On the other hand, > How about you drop use_builtin_ntp and use num_ntp_servers=0 to mean "no bu Sounds good to me. http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/14227/2/src/kudu/mini-cluster/external_mini_cluster.cc@245 PS2, Line 245: // conflicts: chronyd doesn't support binding to wildcard addresses and > As I mentioned in the other patch, I think it'd be better to patch chrony t I think we have discussed this offline and the consensus was to have a patch for chronyd with SO_REUSEPORT for a while. If the upstream maintainers are more willing to accept the ephemeral port bindings instead, we can do that. The reason is that making chronyd working with ephemeral port binding seems to be lot more work than SO_REUSEPORT change. -- To view, visit http://gerrit.cloudera.org:8080/14227 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5c334ae6fa1fb12b033de7f8e8584b8dd3aa2d32 Gerrit-Change-Number: 14227 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 22 Sep 2019 23:46:18 +0000 Gerrit-HasComments: Yes
