Repository: incubator-impala Updated Branches: refs/heads/master 01287a3ba -> 3dff390e4
IMPALA-3682: Don't retry unrecoverable socket creation errors If a thrift client can't create a socket, all subsequent calls to Open() should fail fast since socket creation errors are treated as unrecoverable. Testing: manual testing with a bad SSL configuration. Impalad startup fails fast, rather than retrying 10 times as previously. Change-Id: I394be287143eefc79cf22865898b71ca24c41328 Reviewed-on: http://gerrit.cloudera.org:8080/3317 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Henry Robinson <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/3dff390e Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/3dff390e Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/3dff390e Branch: refs/heads/master Commit: 3dff390e48c19a37e93e952951b060e0f28a73d0 Parents: 01287a3 Author: Henry Robinson <[email protected]> Authored: Mon Jun 6 12:27:03 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Tue Jun 14 10:32:46 2016 -0700 ---------------------------------------------------------------------- be/src/rpc/thrift-client.cc | 5 ++- be/src/rpc/thrift-server-test.cc | 66 ++++++++++++++++++++++-------- be/src/runtime/exec-env.cc | 2 +- common/thrift/generate_error_codes.py | 2 +- 4 files changed, 56 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3dff390e/be/src/rpc/thrift-client.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-client.cc b/be/src/rpc/thrift-client.cc index 2f7d62f..75d231d 100644 --- a/be/src/rpc/thrift-client.cc +++ b/be/src/rpc/thrift-client.cc @@ -46,6 +46,9 @@ Status ThriftClientImpl::Open() { } Status ThriftClientImpl::OpenWithRetry(uint32_t num_tries, uint64_t wait_ms) { + // Socket creation failures are not recoverable. + if (!socket_create_status_.ok()) return socket_create_status_; + uint32_t try_count = 0L; while (true) { ++try_count; @@ -91,7 +94,7 @@ Status ThriftClientImpl::CreateSocket() { ssl_factory_->loadTrustedCertificates(FLAGS_ssl_client_ca_certificate.c_str()); socket_ = ssl_factory_->createSocket(address_.hostname, address_.port); } catch (const TException& e) { - return Status(Substitute("Failed to create socket: $0", e.what())); + return Status(TErrorCode::SSL_SOCKET_CREATION_FAILED, e.what()); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3dff390e/be/src/rpc/thrift-server-test.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc index f8bdf6e..6b010d6 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/be/src/rpc/thrift-server-test.cc @@ -57,19 +57,40 @@ shared_ptr<TProcessor> MakeProcessor() { return shared_ptr<TProcessor>(new StatestoreServiceProcessor(service)); } +int GetServerPort() { + int port = FindUnusedEphemeralPort(); + EXPECT_FALSE(port == -1); + return port; +} + +TEST(ThriftServer, Connectivity) { + int port = GetServerPort(); + ThriftClient<StatestoreServiceClient> wrong_port_client("localhost", + port, "", NULL, false); + ASSERT_FALSE(wrong_port_client.Open().ok()); + + ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, + NULL, 5); + ASSERT_OK(server->Start()); + + // Test that client recovers from failure to connect. + ASSERT_OK(wrong_port_client.Open()); +} + TEST(SslTest, Connectivity) { + int port = GetServerPort(); // Start a server using SSL and confirm that an SSL client can connect, while a non-SSL // client cannot. // Here and elsewhere - allocate ThriftServers on the heap to avoid race during // destruction. See IMPALA-2283. - ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), - FLAGS_state_store_port + 1, NULL, NULL, 5); + ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, + NULL, 5); ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password")); ASSERT_OK(server->Start()); FLAGS_ssl_client_ca_certificate = SERVER_CERT; ThriftClient<StatestoreServiceClient> ssl_client( - "localhost", FLAGS_state_store_port + 1, "", NULL, true); + "localhost", port, "", NULL, true); ASSERT_OK(ssl_client.Open()); TRegisterSubscriberResponse resp; EXPECT_NO_THROW({ @@ -78,23 +99,38 @@ TEST(SslTest, Connectivity) { // Disable SSL for this client. ThriftClient<StatestoreServiceClient> non_ssl_client( - "localhost", FLAGS_state_store_port + 1, "", NULL, false); + "localhost", port, "", NULL, false); ASSERT_OK(non_ssl_client.Open()); EXPECT_THROW(non_ssl_client.iface()->RegisterSubscriber( resp, TRegisterSubscriberRequest()), TTransportException); } +TEST(SslTest, BadCertificate) { + FLAGS_ssl_client_ca_certificate = "unknown"; + int port = GetServerPort(); + ThriftClient<StatestoreServiceClient> ssl_client("localhost", port, "", NULL, true); + ASSERT_FALSE(ssl_client.Open().ok()); + + ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, + NULL, 5); + ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password")); + ASSERT_OK(server->Start()); + + // Check that client does not recover from failure to create socket. + ASSERT_FALSE(ssl_client.Open().ok()); +} + TEST(PasswordProtectedPemFile, CorrectOperation) { // Require the server to execute a shell command to read the password to the private key // file. - ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), - FLAGS_state_store_port + 4, NULL, NULL, 5); + int port = GetServerPort(); + ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, + NULL, 5); ASSERT_OK(server->EnableSsl( SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo password")); ASSERT_OK(server->Start()); FLAGS_ssl_client_ca_certificate = SERVER_CERT; - ThriftClient<StatestoreServiceClient> ssl_client( - "localhost", FLAGS_state_store_port + 4, "", NULL, true); + ThriftClient<StatestoreServiceClient> ssl_client("localhost", port, "", NULL, true); ASSERT_OK(ssl_client.Open()); TRegisterSubscriberResponse resp; EXPECT_NO_THROW({ @@ -104,8 +140,7 @@ TEST(PasswordProtectedPemFile, CorrectOperation) { TEST(PasswordProtectedPemFile, BadPassword) { // Test failure when password to private key is wrong. - ThriftServer server("DummyStatestore", MakeProcessor(), - FLAGS_state_store_port + 2, NULL, NULL, 5); + ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), NULL, NULL, 5); ASSERT_OK(server.EnableSsl( SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo wrongpassword")); EXPECT_FALSE(server.Start().ok()); @@ -113,8 +148,7 @@ TEST(PasswordProtectedPemFile, BadPassword) { TEST(PasswordProtectedPemFile, BadCommand) { // Test failure when password command is badly formed. - ThriftServer server("DummyStatestore", MakeProcessor(), - FLAGS_state_store_port + 3, NULL, NULL, 5); + ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), NULL, NULL, 5); EXPECT_FALSE(server.EnableSsl( SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "cmd-no-exist").ok()); } @@ -122,10 +156,10 @@ TEST(PasswordProtectedPemFile, BadCommand) { TEST(SslTest, ClientBeforeServer) { // Instantiate a thrift client before a thrift server and test if it works (IMPALA-2747) FLAGS_ssl_client_ca_certificate = SERVER_CERT; - ThriftClient<StatestoreServiceClient> ssl_client( - "localhost", FLAGS_state_store_port + 6, "", NULL, true); - ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), - FLAGS_state_store_port + 6, NULL, NULL, 5); + int port = GetServerPort(); + ThriftClient<StatestoreServiceClient> ssl_client("localhost", port, "", NULL, true); + ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, + NULL, 5); ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY)); ASSERT_OK(server->Start()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3dff390e/be/src/runtime/exec-env.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/exec-env.cc b/be/src/runtime/exec-env.cc index 7eaa3db..0260db0 100644 --- a/be/src/runtime/exec-env.cc +++ b/be/src/runtime/exec-env.cc @@ -422,7 +422,7 @@ Status ExecEnv::StartServices() { if (statestore_subscriber_.get() != NULL) { Status status = statestore_subscriber_->Start(); if (!status.ok()) { - status.AddDetail("State Store Subscriber did not start up."); + status.AddDetail("Statestore subscriber did not start up."); return status; } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3dff390e/common/thrift/generate_error_codes.py ---------------------------------------------------------------------- diff --git a/common/thrift/generate_error_codes.py b/common/thrift/generate_error_codes.py index 6c8a621..6365281 100755 --- a/common/thrift/generate_error_codes.py +++ b/common/thrift/generate_error_codes.py @@ -303,7 +303,7 @@ preamble = """ // limitations under the License. // // -// THIS FILE IS AUTO GENERATED BY generated_error_codes.py DO NOT MODIFY +// THIS FILE IS AUTO GENERATED BY generate_error_codes.py DO NOT MODIFY // IT BY HAND. //
