[impala] 10/13: Fail cleanly when in process server can't bind

[ta...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tarmstrong pushed a commit to branch 2.x
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 50cb726c3e48ed247559105a40995e13ef9e7ebb
Author: Tim Armstrong <ta...@cloudera.com>
AuthorDate: Tue Jun 12 17:41:57 2018 -0700

    Fail cleanly when in process server can't bind
    
    This doesn't solve IMPALA-7151 but makes the failure cleaner
    because there is no crash.
    
    Change-Id: I376a2aa559f4b5cf3b96fa3465520e9983ecec4b
    Reviewed-on: http://gerrit.cloudera.org:8080/10726
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/exprs/expr-test.cc             |  8 +++++---
 be/src/service/session-expiry-test.cc |  8 +++++---
 be/src/statestore/statestore-test.cc  |  6 ++++--
 be/src/testutil/in-process-servers.cc | 25 +++++++++----------------
 be/src/testutil/in-process-servers.h  | 12 +++++++-----
 5 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index a96b556..a4ddf15 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -8681,9 +8681,11 @@ int main(int argc, char** argv) {
   FLAGS_abort_on_config_error = false;
   VLOG_CONNECTION << "creating test env";
   VLOG_CONNECTION << "starting backends";
-  InProcessStatestore* ips = InProcessStatestore::StartWithEphemeralPorts();
-  InProcessImpalaServer* impala_server =
-      InProcessImpalaServer::StartWithEphemeralPorts(FLAGS_hostname, ips->port());
+  InProcessStatestore* ips;
+  ABORT_IF_ERROR(InProcessStatestore::StartWithEphemeralPorts(&ips));
+  InProcessImpalaServer* impala_server;
+  ABORT_IF_ERROR(InProcessImpalaServer::StartWithEphemeralPorts(
+      FLAGS_hostname, ips->port(), &impala_server));
   executor_ = new ImpaladQueryExecutor(impala_server->hostname(),
       impala_server->beeswax_port());
   ABORT_IF_ERROR(executor_->Setup());
diff --git a/be/src/service/session-expiry-test.cc b/be/src/service/session-expiry-test.cc
index 9e07b0b..6231b51 100644
--- a/be/src/service/session-expiry-test.cc
+++ b/be/src/service/session-expiry-test.cc
@@ -48,9 +48,11 @@ TEST(SessionTest, TestExpiry) {
   FLAGS_idle_session_timeout = 1;
   // Skip validation checks for in-process backend.
   FLAGS_abort_on_config_error = false;
-  InProcessStatestore* ips = InProcessStatestore::StartWithEphemeralPorts();
-  InProcessImpalaServer* impala =
-      InProcessImpalaServer::StartWithEphemeralPorts("localhost", ips->port());
+  InProcessStatestore* ips;
+  ASSERT_OK(InProcessStatestore::StartWithEphemeralPorts(&ips));
+  InProcessImpalaServer* impala;
+  ASSERT_OK(InProcessImpalaServer::StartWithEphemeralPorts(
+      "localhost", ips->port(), &impala));
   IntCounter* expired_metric =
       impala->metrics()->FindMetricForTesting<IntCounter>(
           ImpaladMetricKeys::NUM_SESSIONS_EXPIRED);
diff --git a/be/src/statestore/statestore-test.cc b/be/src/statestore/statestore-test.cc
index 7f8467f..2e2ccf9 100644
--- a/be/src/statestore/statestore-test.cc
+++ b/be/src/statestore/statestore-test.cc
@@ -37,7 +37,8 @@ namespace impala {
 TEST(StatestoreTest, SmokeTest) {
   // All allocations done by 'new' to avoid problems shutting down Thrift servers
   // gracefully.
-  InProcessStatestore* ips = InProcessStatestore::StartWithEphemeralPorts();
+  InProcessStatestore* ips;
+  ASSERT_OK(InProcessStatestore::StartWithEphemeralPorts(&ips));
   ASSERT_TRUE(ips != NULL) << "Could not start Statestore";
   // Port already in use
   InProcessStatestore* statestore_wont_start =
@@ -69,7 +70,8 @@ TEST(StatestoreSslTest, SmokeTest) {
   server_key << impala_home << "/be/src/testutil/server-key.pem";
   FLAGS_ssl_private_key = server_key.str();
 
-  InProcessStatestore* statestore =  InProcessStatestore::StartWithEphemeralPorts();
+  InProcessStatestore* statestore;
+  ASSERT_OK(InProcessStatestore::StartWithEphemeralPorts(&statestore));
   if (statestore == NULL) FAIL() << "Unable to start Statestore";
 
   vector<int> used_ports;
diff --git a/be/src/testutil/in-process-servers.cc b/be/src/testutil/in-process-servers.cc
index 19afcd5..3ac0777 100644
--- a/be/src/testutil/in-process-servers.cc
+++ b/be/src/testutil/in-process-servers.cc
@@ -42,8 +42,8 @@ DECLARE_int32(krpc_port);
 using namespace apache::thrift;
 using namespace impala;
 
-InProcessImpalaServer* InProcessImpalaServer::StartWithEphemeralPorts(
-    const string& statestore_host, int statestore_port) {
+Status InProcessImpalaServer::StartWithEphemeralPorts(const string& statestore_host,
+    int statestore_port, InProcessImpalaServer** server) {
   for (int tries = 0; tries < 10; ++tries) {
     vector<int> used_ports;
     int backend_port = FindUnusedEphemeralPort(&used_ports);
@@ -68,19 +68,14 @@ InProcessImpalaServer* InProcessImpalaServer::StartWithEphemeralPorts(
     int hs2_port = FindUnusedEphemeralPort(&used_ports);
     if (hs2_port == -1) continue;
 
-    InProcessImpalaServer* impala = new InProcessImpalaServer(FLAGS_hostname,
+    *server = new InProcessImpalaServer(FLAGS_hostname,
         backend_port, krpc_port, subscriber_port, webserver_port, statestore_host,
         statestore_port);
     // Start the daemon and check if it works, if not delete the current server object and
     // pick a new set of ports
-    Status started = impala->StartWithClientServers(beeswax_port, hs2_port);
-    if (started.ok()) return impala;
-    LOG(WARNING) << started.GetDetail();
-
-    delete impala;
+    return (*server)->StartWithClientServers(beeswax_port, hs2_port);
   }
-  DCHECK(false) << "Could not find port to start Impalad.";
-  return NULL;
+  return Status("Could not find ports to start server");
 }
 
 InProcessImpalaServer::InProcessImpalaServer(const string& hostname, int backend_port,
@@ -118,7 +113,7 @@ Status InProcessImpalaServer::Join() {
   return Status::OK();
 }
 
-InProcessStatestore* InProcessStatestore::StartWithEphemeralPorts() {
+Status InProcessStatestore::StartWithEphemeralPorts(InProcessStatestore** statestore) {
   for (int tries = 0; tries < 10; ++tries) {
     vector<int> used_ports;
     int statestore_port = FindUnusedEphemeralPort(&used_ports);
@@ -127,12 +122,10 @@ InProcessStatestore* InProcessStatestore::StartWithEphemeralPorts() {
     int webserver_port = FindUnusedEphemeralPort(&used_ports);
     if (webserver_port == -1) continue;
 
-    InProcessStatestore* ips = new InProcessStatestore(statestore_port, webserver_port);
-    if (ips->Start().ok()) return ips;
-    delete ips;
+    *statestore = new InProcessStatestore(statestore_port, webserver_port);
+    return (*statestore)->Start();
   }
-  DCHECK(false) << "Could not find port to start Statestore.";
-  return NULL;
+  return Status("Could not find port to start Statestore.");
 }
 
 InProcessStatestore::InProcessStatestore(int statestore_port, int webserver_port)
diff --git a/be/src/testutil/in-process-servers.h b/be/src/testutil/in-process-servers.h
index fb69135..ab177ce 100644
--- a/be/src/testutil/in-process-servers.h
+++ b/be/src/testutil/in-process-servers.h
@@ -52,9 +52,10 @@ class InProcessImpalaServer {
   /// member variables. Internally this will call StartWithClientservers() and
   /// SetCatalogInitialized(). The default values for statestore_host and statestore_port
   /// indicate that a statestore connection should not be used. These values are directly
-  /// forwarded to the ExecEnv.
-  static InProcessImpalaServer* StartWithEphemeralPorts(
-      const std::string& statestore_host, int statestore_port);
+  /// forwarded to the ExecEnv. Returns ok and sets *server on success. On failure returns
+  /// an error. *server may or may not be set on error, but is always invalid to use.
+  static Status StartWithEphemeralPorts(const std::string& statestore_host,
+      int statestore_port, InProcessImpalaServer** server);
 
   /// Starts all servers, including the beeswax and hs2 client
   /// servers.
@@ -99,8 +100,9 @@ class InProcessStatestore {
  public:
 
   // Creates and starts an InProcessStatestore with ports chosen from the ephemeral port
-  // range. Returns NULL if no server could be started.
-  static InProcessStatestore* StartWithEphemeralPorts();
+  // range. Returns OK and sets *statestore on success. On failure, an error is
+  /// returned and *statestore may or may not be set but is always invalid to use.
+  static Status StartWithEphemeralPorts(InProcessStatestore** statestore);
 
   /// Constructs but does not start the statestore.
   InProcessStatestore(int statestore_port, int webserver_port);