Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for UNIX domain sockets ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/rpc/acceptor_pool.cc File src/kudu/rpc/acceptor_pool.cc: http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/rpc/acceptor_pool.cc@163 PS7, Line 163: if (remote.is_ip()) { : s = new_sock.SetNoDelay(true); > Nit: Feels like this logic should be abstracted in the Socket class wherein I didn't want to make it silently a no-op in the Socket class -- that would give people the wrong sense that it actually does something. It could return NotSupported (in fact it might already do so by virtue of interpreting errno?) but then we still need this code here, so it doesn't simplify anything. http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/sockaddr.h@50 PS7, Line 50: sockaddr &addr > nit: & next to sockaddr Done http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/sockaddr.h@111 PS7, Line 111: UnixPath > Nit: Should the function be named UnixDomainPath() to be consistent with Pa Done http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/sockaddr.cc@204 PS7, Line 204: #ifndef NDEBUG : CHECK_EQ(family(), AF_INET); : #else : if (family() != AF_INET) { : // In case we missed a host() call somewhere in a vlog or error message not : // covered by tests, better to at least return some string here. : return "<unix socket>"; : } : #endif : return HostPort::AddrToString(storage_.in.sin_addr.s_addr); > Wouldn't it be better to use switch case on family() and return appropriate Done http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc@102 PS7, Line 102: void DoUnixSocketTest(const string& path); > Nit: Seems odd for definition to be separate for this just one function. Done http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc@128 PS7, Line 128: DoUnixSocketTest > This test transfers from server to client. Can the test be updated to trans after connecting, a connection is symmetric, so wanted to keep the test as short/simple as possible. Both directions of data transfer will be covered well by larger tests (rpc layer) so I don't think we need the extra coverage here. http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc@129 PS7, Line 129: const > static const/constexpr I generally try to avoid static lifetime non-POD structures (string is not a POD), because there is all kinds of weirdness about when constructors/destructors run. And it's also not constexpr because it's non-POD http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc@141 PS7, Line 141: "unix:<unnamed>" > Curious why is peer address from server "unnamed"? the client didn't bind to any path before connecting http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket.cc@139 PS7, Line 139: family > Should we validate supported family options and return appropriate Status e the underlying socket() API will return an error code in that case, and we'll return NetworkError appropriately http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket.cc@151 PS7, Line 151: Reset(::socket(family, SOCK_STREAM, 0)); > Same here. see above -- To view, visit http://gerrit.cloudera.org:8080/15691 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b Gerrit-Change-Number: 15691 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Comment-Date: Wed, 15 Apr 2020 16:31:00 +0000 Gerrit-HasComments: Yes
