Adar Dembo has posted comments on this change. Change subject: Use dynamic bind address for internal + external mini clusters ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/7274/1//COMMIT_MSG Commit Message: Line 7: Use dynamic bind address for internal + external mini clusters I'd reword this summary to explain in more detail what's actually changing, which as far as I can tell, is: 1. Applying the "dynamic bind address" technique to internal mini clusters (previously it was only used for external mini clusters). 2. Applying it to masters (previously it was only used for tservers). I think this is an important change; doesn't it mean we can safely drop RUN_SERIAL from the various multi-master integration tests? Or will that gum up the works on macOS? http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: Line 305: if (opts_.enable_kerberos) { So we put the KDC on the same interface as the master? Probably worth a comment. http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 457: // Get/Set rpc_bind_addresses for daemon. Why are these defined as virtual? Why do we need a setter? Can't we set rpc_bind_address_ in the constructor? Ideally we'd pass it down to the ExternalDaemon constructor, which would set it there. Line 534: HostPort rpc_bind_addr, Nit: use the same name as the ExternalMaster constructor. http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/internal_mini_cluster.cc File src/kudu/integration-tests/internal_mini_cluster.cc: Line 51: #if defined(__APPLE__) Must we redefine this for InternalMiniCluster and ExternalMiniCluster? Seems like the base class could declare it in the header, and switch the #define in its .cc. http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/internal_mini_cluster.h File src/kudu/integration-tests/internal_mini_cluster.h: Line 66: // Defaults to a list of 0 (ephemeral ports). "List of 0" is confusing; how about "empty list"? http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/mini_cluster.cc File src/kudu/integration-tests/mini_cluster.cc: Line 33: vector<HostPort> MiniCluster::master_rpc_addrs() const { I found it frustrating to navigate from the definitions of StartDistributedMasters() down to this class to read this common definition, then to navigate back to the superclasses. Since the function is short, consider duplicating it in the superclasses instead. PS1, Line 34: BindMode bind_mode = this->bind_mode(); : vector<uint16_t> master_rpc_ports = this->master_rpc_ports(); Nit: these local variables seem unnecessary; they don't really shorten anything. PS1, Line 48: UINT8_MAX > This should be (UINT8_MAX - 1). Doesn't DCHECK_LT(index, UINT8_MAX) guarantee that index will never equal UINT8_MAX, though? It's LT, not LE. http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/integration-tests/mini_cluster.h File src/kudu/integration-tests/mini_cluster.h: Line 122: virtual std::vector<HostPort> master_rpc_addrs() const; Kind of annoying that it's an abstract base class except for this one method. It's easy to overlook it. http://gerrit.cloudera.org:8080/#/c/7274/1/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: Line 106: Status MiniMaster::StartOnPorts(const string& bind_host, uint16_t rpc_port, uint16_t web_port, Not your fault, but it seems odd that this (and StartDistributedMasterOnPorts) basically take pre-split members as arguments. Why not just reference the members directly in the functions, splitting host/port as needed there? -- To view, visit http://gerrit.cloudera.org:8080/7274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35eff9fbf5ccf8822cfe061673bc36598dff56f0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
