Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15701 )
Change subject: client/tserver: add support for connecting over unix domain sockets ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/15701/2//COMMIT_MSG Commit Message: PS2: Do you think it's worth adding a test to enable these in a mini cluster and client? http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/client/meta_cache.h@128 PS2, Line 128: boost::optional<std::string> unix_domain_socket_path_; nit: should comment that we need to check that we're local before using this. Or only set this if we're local. http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/client/meta_cache.cc@83 PS2, Line 83: thta nit: typo http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/client/meta_cache.cc@151 PS2, Line 151: << "Tablet server " << hp.ToString() << "(" << uuid_ << ") " : << " reported an invalid unix domain socket path " << *unix_domain_socket_path_; nit: extra space after the parens. Prefer Substitute() for readability? Same below http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/common/wire_protocol.cc@203 PS2, Line 203: // Don't add unix domain sockets to the list of HostPorts. : if (!addr.is_ip()) continue; Do you think we'll ever want to have master-client transfer over domain sockets? Maybe consider a version of this that fills in parts of a ServerRegistrationPB http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/common/wire_protocol.proto@101 PS2, Line 101: an nit: capitalize http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/master/master.proto@436 PS2, Line 436: an nit: capitalize http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/server/rpc_server.cc File src/kudu/server/rpc_server.cc: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/server/rpc_server.cc@178 PS2, Line 178: if (server_state_ != INITIALIZED) { : return Status::IllegalState("must add bound addresses between Init() and Bind()"); : } nit: if this is a programmer error, consider CHECK_NE? http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/15701/2/src/kudu/server/server_base.cc@541 PS2, Line 541: CHECK_OK(addr.ParseUnixDomainPath( : Substitute("@kudu-$0", fs_manager_->uuid()))); : CHECK_OK(rpc_server_->AddBindAddress(addr)); Why not RETURN_NOT_OK for these? -- To view, visit http://gerrit.cloudera.org:8080/15701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c390b4209ac7e08cd45239c49499fb0b96405d0 Gerrit-Change-Number: 15701 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Sun, 12 Apr 2020 08:20:24 +0000 Gerrit-HasComments: Yes
