You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/12/06 20:56:00 UTC
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5385
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
IMPALA-2864: Ensure that client connections are closed after a failed Open()
When a client tries to Open() a socket and fails, we
previously assumed that the socket was never opened and therefore did
not close it. However, if Kerberos or SSL is enabled, the
ThriftClientImpl::Open() calls TSaslTransport::Open(), which not only
opens the socket but also does the initial handshake. If there was an
error during the handshake, we just returned with an error without
closing the socket, causing the server side to wait on the same
connection.
This patch closes the client side socket, thereby terminating the
connection to the server in the above scenario, so that the server
side doesn't need to hold on to a connection until a timeout
terminates the connection.
A thrift-server-test is added to test the RPC failure path.
Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/client-cache.h
M be/src/runtime/data-stream-test.cc
A be/src/testutil/bad-cert.pem
A be/src/testutil/bad-key.pem
7 files changed, 85 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/5385/1
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
IMPALA-2864: Ensure that client connections are closed after a failed Open()
When a client tries to Open() a socket and fails, we
previously assumed that the socket was never opened and therefore did
not close it. However, if Kerberos is enabled, the
ThriftClientImpl::Open() calls TSaslTransport::Open(), which not only
opens the socket but also does the initial handshake. If there was an
error during the handshake, we just returned with an error without
closing the socket, causing the server side to wait on the same
connection.
This patch closes the client side socket, thereby terminating the
connection to the server in the above scenario, so that the server
side doesn't need to hold on to a connection until a timeout
terminates the connection.
A thrift-server-test is added to test the RPC failure path.
Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/client-cache.h
M be/src/runtime/data-stream-test.cc
A be/src/testutil/bad-cert.pem
A be/src/testutil/bad-key.pem
7 files changed, 85 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/5385/2
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:
PS2, Line 46: socket
confusing since it talks about "socket' when the code does something to the transport. How about just removing this comment? It seems redundant with the C++ code.
http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:
Line 55: /// We also ensure that a failed Open() leaves the socket in a closed state.
Is "connection" and "socket" synomymous, or not? Everwhere else here we say connection. If so, let's say connection.
An let's be more direct about the post condition (rather than what we do to implement that):
If Open() fails, the connection remains closed.
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
Rebase. Carry +2.
http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.cc
File be/src/rpc/thrift-client.cc:
PS2, Line 46:
> confusing since it talks about "socket' when the code does something to the
Done
http://gerrit.cloudera.org:8080/#/c/5385/2/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:
Line 55: /// If Open() fails, the connection remains closed.
> Is "connection" and "socket" synomymous, or not? Everwhere else here we say
They are synonymous in this context at least. Done.
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5385
to look at the new patch set (#3).
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
IMPALA-2864: Ensure that client connections are closed after a failed Open()
When a client tries to Open() a socket and fails, we
previously assumed that the socket was never opened and therefore did
not close it. However, if Kerberos is enabled, the
ThriftClientImpl::Open() calls TSaslTransport::Open(), which not only
opens the socket but also does the initial handshake. If there was an
error during the handshake, we just returned with an error without
closing the socket, causing the server side to wait on the same
connection.
This patch closes the client side socket, thereby terminating the
connection to the server in the above scenario, so that the server
side doesn't need to hold on to a connection until a timeout
terminates the connection.
A thrift-server-test is added to test the RPC failure path.
Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/client-cache.h
M be/src/runtime/data-stream-test.cc
A be/src/testutil/bad-cert.pem
A be/src/testutil/bad-key.pem
7 files changed, 84 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/5385/3
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-2864: Ensure that client connections are closed after a failed Open()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-2864: Ensure that client connections are closed after a failed Open()
......................................................................
IMPALA-2864: Ensure that client connections are closed after a failed Open()
When a client tries to Open() a socket and fails, we
previously assumed that the socket was never opened and therefore did
not close it. However, if Kerberos is enabled, the
ThriftClientImpl::Open() calls TSaslTransport::Open(), which not only
opens the socket but also does the initial handshake. If there was an
error during the handshake, we just returned with an error without
closing the socket, causing the server side to wait on the same
connection.
This patch closes the client side socket, thereby terminating the
connection to the server in the above scenario, so that the server
side doesn't need to hold on to a connection until a timeout
terminates the connection.
A thrift-server-test is added to test the RPC failure path.
Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Reviewed-on: http://gerrit.cloudera.org:8080/5385
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/runtime/client-cache.h
M be/src/runtime/data-stream-test.cc
A be/src/testutil/bad-cert.pem
A be/src/testutil/bad-key.pem
7 files changed, 84 insertions(+), 2 deletions(-)
Approvals:
Internal Jenkins: Verified
Sailesh Mukil: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5385
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>