You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/04/09 00:06:41 UTC
[kudu-CR] WIP: Add basic support for unix sockets
Todd Lipcon has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15691
Change subject: WIP: Add basic support for unix sockets
......................................................................
WIP: Add basic support for unix sockets
so far can't seem to reproduce any real speedup
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
17 files changed, 318 insertions(+), 97 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/1
--
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: newchange
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic support for unix domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#3).
Change subject: Add basic support for unix domain sockets
......................................................................
Add basic support for unix domain sockets
This adds support to Sockaddr to represent unix domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and Unix
sockets. It also adds a simple performance test of running RPC over a
unix domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
24 files changed, 551 insertions(+), 258 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/3
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 5:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/connection.cc@908
PS4, Line 908: is_ip()) {
> nit: use is_ip() here? Also, is the idea here that most of the stats are TC
yea, the getsockopt call is TCP-specific
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/rpc-test.cc@1535
PS4, Line 1535: GenericCalculatorService::static_service_name());
> nit: aren't these the same? Also, would be easier to read without the negat
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.h@121
PS4, Line 121: xample, the addre
> nit: comment needs an update
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@22
PS4, Line 22: #include <sys/socket.h>
> nit: typically prefer cstddef in new code
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@103
PS4, Line 103: str
> nit: drop the std
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@120
PS4, Line 120: UNIX
> nit: lower case for consistency
going to upper-case it elsewhere since the manpage calls is UNIX domain sockets
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc
File src/kudu/util/net/socket-test.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc@20
PS4, Line 20: #include <unistd.h>
> nit: here too
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc@114
PS4, Line 114: DoUnixSocketTest(strings::Substitute("@kudu-test-pid-$0", getpid()));
> nit: Is it worth testing pathname and abstract addresses?
Done
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket.cc@325
PS4, Line 325: bind_addr.is_ip() &&
> nit: using is_ip() might be more self-documenting here
Done
http://gerrit.cloudera.org:8080/#/c/15691/5/src/kudu/util/thread-test.cc
File src/kudu/util/thread-test.cc:
http://gerrit.cloudera.org:8080/#/c/15691/5/src/kudu/util/thread-test.cc@166
PS5, Line 166: std::atomic<int> finished { 0 };
> error: no member named 'atomic' in namespace 'std'; did you mean 'atoi'? [c
oops I didn't mean to commit this here...
--
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: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 17:26:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Andrew Wong, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#6).
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 565 insertions(+), 261 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/6
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for UNIX domain sockets
......................................................................
Patch Set 7: Code-Review+1
(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 SetNoDelay() action is only taken on IP sockets.
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
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 ParseUnixDomainPath() above?
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 host string ?
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.
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 transfer from client to server as well?
http://gerrit.cloudera.org:8080/#/c/15691/7/src/kudu/util/net/socket-test.cc@129
PS7, Line 129: const
static const/constexpr
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"?
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 error?
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.
--
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 <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Apr 2020 00:35:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic support for unix domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#2).
Change subject: Add basic support for unix domain sockets
......................................................................
Add basic support for unix domain sockets
This adds support to Sockaddr to represent unix domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and Unix
sockets. It also adds a simple performance test of running RPC over a
unix domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
24 files changed, 538 insertions(+), 257 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/2
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for unix domain sockets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for unix domain sockets
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/connection.cc@908
PS4, Line 908: family() != AF_UNIX
nit: use is_ip() here? Also, is the idea here that most of the stats are TCP-related and don't make sense for unix sockets? A comment here would be nice.
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/rpc/rpc-test.cc@1535
PS4, Line 1535: !use_unix_socket() ? kRemoteHostName : "localhost",
nit: aren't these the same? Also, would be easier to read without the negative.
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.h@121
PS4, Line 121: unix_address_type
nit: comment needs an update
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@22
PS4, Line 22: #include <stddef.h>
nit: typically prefer cstddef in new code
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@103
PS4, Line 103: std
nit: drop the std
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/sockaddr.cc@120
PS4, Line 120: UNIX
nit: lower case for consistency
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc
File src/kudu/util/net/socket-test.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc@20
PS4, Line 20: #include <stddef.h>
nit: here too
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket-test.cc@114
PS4, Line 114: string path = strings::Substitute("@kudu-test-pid-$0", getpid());
nit: Is it worth testing pathname and abstract addresses?
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/15691/4/src/kudu/util/net/socket.cc@325
PS4, Line 325: bind_addr.family() != AF_UNIX
nit: using is_ip() might be more self-documenting here
--
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: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sun, 12 Apr 2020 07:37:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add basic support for unix sockets
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: WIP: Add basic support for unix sockets
......................................................................
Patch Set 1: Code-Review+1
--
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: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 07:10:57 +0000
Gerrit-HasComments: No
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Andrew Wong, Grant Henke, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#7).
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 573 insertions(+), 261 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/7
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for UNIX domain sockets
......................................................................
Patch Set 7: Code-Review+2
--
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 <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 23:58:05 +0000
Gerrit-HasComments: No
[kudu-CR] Add basic support for unix domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#4).
Change subject: Add basic support for unix domain sockets
......................................................................
Add basic support for unix domain sockets
This adds support to Sockaddr to represent unix domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and Unix
sockets. It also adds a simple performance test of running RPC over a
unix domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 551 insertions(+), 259 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/4
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Andrew Wong, Grant Henke, Volodymyr Verovkin, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#8).
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 575 insertions(+), 264 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/8
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Andrew Wong, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#5).
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
M src/kudu/util/thread-test.cc
26 files changed, 584 insertions(+), 261 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/5
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Andrew Wong, Kudu Jenkins, Andrew Wong, Grant Henke, Volodymyr Verovkin, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15691
to look at the new patch set (#9).
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 576 insertions(+), 264 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15691/9
--
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: newpatchset
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 9
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 9: Code-Review+2
Forwarding Andrew's +2
--
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: 9
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Apr 2020 22:10:39 +0000
Gerrit-HasComments: No
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for UNIX domain sockets
......................................................................
Add basic support for UNIX domain sockets
This adds support to Sockaddr to represent UNIX domain sockets, and
fixes some code in the RPC stack and Socket classes that previously
assumed sockaddr_in.
This updates rpc-test to run all of the cases over both TCP and UNIX
sockets. It also adds a simple performance test of running RPC over a
UNIX domain socket.
The performance difference is small but measurable. The test has a lot
of variability depending on whether the reactor threads, test thread,
and RPC worker thread get scheduled to the same core, different
hyperthreads of the same core, same vs different NUMA nodes, etc. To
eliminate that variability I ran the performance test using 'taskset -c
1' to pin to a single CPU:
I0409 15:15:50.290123 42405 rpc-test.cc:1529] Connecting to 0.0.0.0:33635
I0409 15:15:50.646266 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.355s user 0.110s sys 0.245s
I0409 15:15:51.048060 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.402s user 0.106s sys 0.295s
I0409 15:15:51.455765 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.408s user 0.120s sys 0.287s
I0409 15:15:51.861732 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.406s user 0.112s sys 0.293s
I0409 15:15:52.268314 42405 rpc-test.cc:1543] Sending 1024MB via tcp socket: real 0.407s user 0.119s sys 0.287s
[ OK ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_TCP (1987 ms)
[ RUN ] Parameters/TestRpc.TestPerformanceBySocketType/NoSSL_UnixSocket
I0409 15:15:52.271142 42405 rpc-test.cc:1529] Connecting to unix:@kudu-test-42405
I0409 15:15:52.605296 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.333s user 0.098s sys 0.235s
I0409 15:15:52.934170 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.329s user 0.107s sys 0.222s
I0409 15:15:53.264912 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.331s user 0.115s sys 0.216s
I0409 15:15:53.592478 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.328s user 0.115s sys 0.212s
I0409 15:15:53.918494 42405 rpc-test.cc:1543] Sending 1024MB via unix socket: real 0.326s user 0.107s sys 0.219s
The results show about a 1.2x throughput improvement using unix sockets.
Another benefit we can make use of in the future is the ability to pass file
descriptors over sockets: this would enable us to do true "zero-copy" between
the server and client, which should have more significant benefits than seen
here.
Additionally, this patch will probably help get us closer to IPv6 support as
well, since it gets rid of some sockaddr_in-specific code paths in the RPC
stack.
Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Reviewed-on: http://gerrit.cloudera.org:8080/15691
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/acceptor_pool.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
M src/kudu/util/net/socket-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
25 files changed, 576 insertions(+), 264 deletions(-)
Approvals:
Bankim Bhavsar: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I40ce3e4e1b98f806f0c29d2fbd88789657218c4b
Gerrit-Change-Number: 15691
Gerrit-PatchSet: 10
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15691 )
Change subject: Add basic support for UNIX domain sockets
......................................................................
Patch Set 9: Code-Review+1
--
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: 9
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Apr 2020 18:33:15 +0000
Gerrit-HasComments: No
[kudu-CR] Add basic support for UNIX domain sockets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Apr 2020 16:31:00 +0000
Gerrit-HasComments: Yes