kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: Miscellaneous cleanup
Date Tue, 17 Oct 2017 01:18:24 GMT
Repository: kudu
Updated Branches:
  refs/heads/master ab77ce025 -> 29514389a


Miscellaneous cleanup

A collection of small cleanup changes that I don't want to lump in with
a bigger feature patch.

Change-Id: I94a1f5d476d30a959a056040b3eb30dfebcc9378
Reviewed-on: http://gerrit.cloudera.org:8080/8268
Reviewed-by: Adar Dembo <adar@cloudera.com>
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
Tested-by: Kudu Jenkins


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/29514389
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/29514389
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/29514389

Branch: refs/heads/master
Commit: 29514389ab629a9827c40b9d08acb378adc44801
Parents: ab77ce0
Author: Dan Burkert <danburkert@apache.org>
Authored: Thu Oct 12 18:07:11 2017 -0700
Committer: Dan Burkert <danburkert@apache.org>
Committed: Tue Oct 17 01:17:09 2017 +0000

----------------------------------------------------------------------
 src/kudu/master/CMakeLists.txt     |  5 ++-
 src/kudu/master/catalog_manager.cc | 16 ++++++---
 src/kudu/master/catalog_manager.h  |  2 +-
 src/kudu/master/master-test.cc     | 64 ++++++++++++++-------------------
 src/kudu/master/sys_catalog.cc     |  6 ----
 src/kudu/master/sys_catalog.h      |  2 +-
 src/kudu/rpc/connection.cc         |  6 ++--
 src/kudu/rpc/outbound_call.cc      | 16 ++++-----
 src/kudu/rpc/outbound_call.h       |  9 +++--
 src/kudu/rpc/reactor.cc            |  2 +-
 10 files changed, 58 insertions(+), 70 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/master/CMakeLists.txt b/src/kudu/master/CMakeLists.txt
index 1100807..53fbbde 100644
--- a/src/kudu/master/CMakeLists.txt
+++ b/src/kudu/master/CMakeLists.txt
@@ -34,16 +34,15 @@ ADD_EXPORTABLE_LIBRARY(master_proto
 
 set(MASTER_SRCS
   catalog_manager.cc
+  master-path-handlers.cc
   master.cc
   master_cert_authority.cc
   master_options.cc
   master_service.cc
-  master-path-handlers.cc
   mini_master.cc
   sys_catalog.cc
   ts_descriptor.cc
-  ts_manager.cc
-)
+  ts_manager.cc)
 
 add_library(master ${MASTER_SRCS})
 target_link_libraries(master

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 1958398..4f0a6d5 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -1347,9 +1347,14 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
 
     // c. Reserve the table name if possible.
     if (!InsertIfNotPresent(&reserved_table_names_, req.name())) {
+      // ServiceUnavailable will cause the client to retry the create table
+      // request. We don't want to outright fail the request with
+      // 'AlreadyPresent', because a table name reservation can be rolled back
+      // in the case of an error. Instead, we force the client to retry at a
+      // later time.
       s = Status::ServiceUnavailable(Substitute(
           "New table name $0 is already reserved", req.name()));
-      return SetError(MasterErrorPB::TABLE_NOT_FOUND, s);
+      return SetError(MasterErrorPB::TABLE_ALREADY_PRESENT, s);
     }
   }
 
@@ -1884,8 +1889,6 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
   LOG(INFO) << Substitute("Servicing AlterTable request from $0:\n$1",
                           RequestorString(rpc), SecureShortDebugString(*req));
 
-  RETURN_NOT_OK(CheckOnline());
-
   // 1. Group the steps into schema altering steps and partition altering steps.
   vector<AlterTableRequestPB::Step> alter_schema_steps;
   vector<AlterTableRequestPB::Step> alter_partitioning_steps;
@@ -1973,9 +1976,14 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
 
     // Reserve the new table name if possible.
     if (!InsertIfNotPresent(&reserved_table_names_, req->new_table_name())) {
+      // ServiceUnavailable will cause the client to retry the create table
+      // request. We don't want to outright fail the request with
+      // 'AlreadyPresent', because a table name reservation can be rolled back
+      // in the case of an error. Instead, we force the client to retry at a
+      // later time.
       Status s = Status::ServiceUnavailable(Substitute(
           "Table name $0 is already reserved", req->new_table_name()));
-      SetupError(resp->mutable_error(), MasterErrorPB::TABLE_NOT_FOUND, s);
+      SetupError(resp->mutable_error(), MasterErrorPB::TABLE_ALREADY_PRESENT, s);
       return s;
     }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/catalog_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index aab38dc..b25a655 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -88,8 +88,8 @@ namespace master {
 class CatalogManagerBgTasks;
 class Master;
 class SysCatalogTable;
-class TableInfo;
 class TSDescriptor;
+class TableInfo;
 
 struct DeferredAssignmentActions;
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/master-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index 9656dc3..30a914d 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -1159,23 +1159,19 @@ TEST_F(MasterTest, TestConcurrentCreateOfSameTable) {
       // There are three expected outcomes:
       //
       // 1. This thread won the CreateTable() race: no error.
-      // 2. This thread lost the CreateTable() race: TABLE_NOT_FOUND error
-      //    with ServiceUnavailable status.
+      // 2. This thread lost the CreateTable() race, but the table is still
+      //    in the process of being created: TABLE_ALREADY_PRESENT error with
+      //    ServiceUnavailable status.
       // 3. This thread arrived after the CreateTable() race was already over:
       //    TABLE_ALREADY_PRESENT error with AlreadyPresent status.
       if (resp.has_error()) {
         Status s = StatusFromPB(resp.error().status());
         string failure_msg = Substitute("Unexpected response: $0",
                                         SecureDebugString(resp));
-        switch (resp.error().code()) {
-          case MasterErrorPB::TABLE_NOT_FOUND:
-            CHECK(s.IsServiceUnavailable()) << failure_msg;
-            break;
-          case MasterErrorPB::TABLE_ALREADY_PRESENT:
-            CHECK(s.IsAlreadyPresent()) << failure_msg;
-            break;
-          default:
-            FAIL() << failure_msg;
+        if (resp.error().code() == MasterErrorPB::TABLE_ALREADY_PRESENT) {
+          CHECK(s.IsServiceUnavailable() || s.IsAlreadyPresent()) << failure_msg;
+        } else {
+          FAIL() << failure_msg;
         }
       }
     });
@@ -1265,23 +1261,17 @@ TEST_F(MasterTest, TestConcurrentCreateAndRenameOfSameTable) {
         // There are three expected outcomes:
         //
         // 1. This thread finished well before the others: no error.
-        // 2. This thread raced with another thread: TABLE_NOT_FOUND error with
-        //    ServiceUnavailable status.
-        // 3. This thread finished well after the others: TABLE_ALREADY_PRESENT
-        //    error with AlreadyPresent status.
+        // 2. This thread raced with CreateTable() or AlterTable():
+        //    TABLE_ALREADY_PRESENT error with ServiceUnavailable status.
+        // 3. This thread finished well after the others:
+        //    TABLE_ALREADY_PRESENT error with AlreadyPresent status.
         if (resp.has_error()) {
           Status s = StatusFromPB(resp.error().status());
-          string failure_msg = Substitute("Unexpected response: $0",
-                                          SecureDebugString(resp));
-          switch (resp.error().code()) {
-            case MasterErrorPB::TABLE_NOT_FOUND:
-              CHECK(s.IsServiceUnavailable()) << failure_msg;
-              break;
-            case MasterErrorPB::TABLE_ALREADY_PRESENT:
-              CHECK(s.IsAlreadyPresent()) << failure_msg;
-              break;
-            default:
-              FAIL() << failure_msg;
+          string failure_msg = Substitute("Unexpected response: $0", SecureDebugString(resp));
+          if (resp.error().code() == MasterErrorPB::TABLE_ALREADY_PRESENT) {
+              CHECK(s.IsServiceUnavailable() || s.IsAlreadyPresent()) << failure_msg;
+          } else {
+            FAIL() << failure_msg;
           }
         } else {
           // Creating the table should only succeed once.
@@ -1299,25 +1289,25 @@ TEST_F(MasterTest, TestConcurrentCreateAndRenameOfSameTable) {
         CHECK_OK(proxy_->AlterTable(req, &resp, &controller));
         SCOPED_TRACE(SecureDebugString(resp));
 
-        // There are three expected outcomes:
+        // There are four expected outcomes:
         //
         // 1. This thread finished well before the others: no error.
-        // 2. This thread raced with CreateTable(): TABLE_NOT_FOUND error with
-        //    ServiceUnavailable status (if raced during reservation stage)
-        //    or TABLE_ALREADY_PRESENT error with AlreadyPresent status (if
-        //    raced after reservation stage).
-        // 3. This thread raced with AlterTable() or finished well after the
-        //    others: TABLE_NOT_FOUND error with NotFound status.
+        // 2. This thread raced with CreateTable() or AlterTable():
+        //    TABLE_ALREADY_PRESENT error with ServiceUnavailable status.
+        // 3. This thread completed well after a CreateTable():
+        //    TABLE_ALREADY_PRESENT error with AlreadyPresent status.
+        // 4. This thread completed well after an AlterTable():
+        //    TABLE_NOT_FOUND error with NotFound status.
         if (resp.has_error()) {
           Status s = StatusFromPB(resp.error().status());
           string failure_msg = Substitute("Unexpected response: $0",
                                           SecureDebugString(resp));
           switch (resp.error().code()) {
-            case MasterErrorPB::TABLE_NOT_FOUND:
-              CHECK(s.IsServiceUnavailable() || s.IsNotFound()) << failure_msg;
-              break;
             case MasterErrorPB::TABLE_ALREADY_PRESENT:
-              CHECK(s.IsAlreadyPresent()) << failure_msg;
+              CHECK(s.IsServiceUnavailable() || s.IsAlreadyPresent()) << failure_msg;
+              break;
+            case MasterErrorPB::TABLE_NOT_FOUND:
+              CHECK(s.IsNotFound()) << failure_msg;
               break;
             default:
               FAIL() << failure_msg;

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/sys_catalog.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index e7db525..13ebfd6 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -469,12 +469,6 @@ Schema SysCatalogTable::BuildTableSchema() {
   return builder.Build();
 }
 
-SysCatalogTable::Actions::Actions()
-    : table_to_add(nullptr),
-      table_to_update(nullptr),
-      table_to_delete(nullptr) {
-}
-
 Status SysCatalogTable::Write(const Actions& actions) {
   TRACE_EVENT0("master", "SysCatalogTable::Write");
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/master/sys_catalog.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/sys_catalog.h b/src/kudu/master/sys_catalog.h
index 5ea5493..49ae07f 100644
--- a/src/kudu/master/sys_catalog.h
+++ b/src/kudu/master/sys_catalog.h
@@ -144,7 +144,7 @@ class SysCatalogTable {
 
   // Perform a series of table/tablet actions in one WriteTransaction.
   struct Actions {
-    Actions();
+    Actions() = default;
 
     scoped_refptr<TableInfo> table_to_add;
     scoped_refptr<TableInfo> table_to_update;

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/rpc/connection.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/connection.cc b/src/kudu/rpc/connection.cc
index d9ac03b..e4a9245 100644
--- a/src/kudu/rpc/connection.cc
+++ b/src/kudu/rpc/connection.cc
@@ -167,7 +167,7 @@ void Connection::Shutdown(const Status &status,
       c->call->SetFailed(status,
                          negotiation_complete_ ? Phase::REMOTE_CALL
                                                : Phase::CONNECTION_NEGOTIATION,
-                         error.release());
+                         std::move(error));
     }
     // And we must return the CallAwaitingResponse to the pool
     car_pool_.Destroy(c);
@@ -635,8 +635,8 @@ void Connection::WriteHandler(ev::io &watcher, int revents) {
           outbound_transfers_.pop_front();
           Status s = Status::NotSupported("server does not support the required RPC features");
           transfer->Abort(s);
-          car->call->SetFailed(s, negotiation_complete_ ? Phase::REMOTE_CALL
-                                                        : Phase::CONNECTION_NEGOTIATION);
+          Phase phase = negotiation_complete_ ? Phase::REMOTE_CALL : Phase::CONNECTION_NEGOTIATION;
+          car->call->SetFailed(std::move(s), phase);
           // Test cancellation when 'call_' is in 'FINISHED_ERROR' state.
           MaybeInjectCancellation(car->call);
           car->call.reset();

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/rpc/outbound_call.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/outbound_call.cc b/src/kudu/rpc/outbound_call.cc
index d4ca78b..aac06eb 100644
--- a/src/kudu/rpc/outbound_call.cc
+++ b/src/kudu/rpc/outbound_call.cc
@@ -305,14 +305,14 @@ void OutboundCall::SetResponse(gscoped_ptr<CallResponse> resp)
{
     CallCallback();
   } else {
     // Error
-    gscoped_ptr<ErrorStatusPB> err(new ErrorStatusPB());
+    unique_ptr<ErrorStatusPB> err(new ErrorStatusPB());
     if (!err->ParseFromArray(r.data(), r.size())) {
       SetFailed(Status::IOError("Was an RPC error but could not parse error response",
                                 err->InitializationErrorString()));
       return;
     }
-    ErrorStatusPB* err_raw = err.release();
-    SetFailed(Status::RemoteError(err_raw->message()), Phase::REMOTE_CALL, err_raw);
+    Status s = Status::RemoteError(err->message());
+    SetFailed(std::move(s), Phase::REMOTE_CALL, std::move(err));
   }
 }
 
@@ -344,17 +344,15 @@ void OutboundCall::SetSent() {
   }
 }
 
-void OutboundCall::SetFailed(const Status &status,
+void OutboundCall::SetFailed(Status status,
                              Phase phase,
-                             ErrorStatusPB* err_pb) {
+                             unique_ptr<ErrorStatusPB> err_pb) {
   DCHECK(!status.ok());
   DCHECK(phase == Phase::CONNECTION_NEGOTIATION || phase == Phase::REMOTE_CALL);
   {
     std::lock_guard<simple_spinlock> l(lock_);
-    status_ = status;
-    if (err_pb) {
-      error_pb_.reset(err_pb);
-    }
+    status_ = std::move(status);
+    error_pb_ = std::move(err_pb);
     set_state_unlocked(phase == Phase::CONNECTION_NEGOTIATION
         ? FINISHED_NEGOTIATION_ERROR
         : FINISHED_ERROR);

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/rpc/outbound_call.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/outbound_call.h b/src/kudu/rpc/outbound_call.h
index 957c005..a47704e 100644
--- a/src/kudu/rpc/outbound_call.h
+++ b/src/kudu/rpc/outbound_call.h
@@ -128,11 +128,10 @@ class OutboundCall {
 
   // Mark the call as failed. This also triggers the callback to notify
   // the caller. If the call failed due to a remote error, then err_pb
-  // should be set to the error returned by the remote server. Takes
-  // ownership of 'err_pb'.
-  void SetFailed(const Status& status,
+  // should be set to the error returned by the remote server.
+  void SetFailed(Status status,
                  Phase phase = Phase::REMOTE_CALL,
-                 ErrorStatusPB* err_pb = nullptr);
+                 std::unique_ptr<ErrorStatusPB> err_pb = nullptr);
 
   // Mark the call as timed out. This also triggers the callback to notify
   // the caller.
@@ -238,7 +237,7 @@ class OutboundCall {
   mutable simple_spinlock lock_;
   State state_;
   Status status_;
-  gscoped_ptr<ErrorStatusPB> error_pb_;
+  std::unique_ptr<ErrorStatusPB> error_pb_;
 
   // Call the user-provided callback. Note that entries in 'sidecars_' are cleared
   // prior to invoking the callback so the client can assume that the call doesn't

http://git-wip-us.apache.org/repos/asf/kudu/blob/29514389/src/kudu/rpc/reactor.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/reactor.cc b/src/kudu/rpc/reactor.cc
index 8eb096f..c9cd92a 100644
--- a/src/kudu/rpc/reactor.cc
+++ b/src/kudu/rpc/reactor.cc
@@ -337,7 +337,7 @@ void ReactorThread::AssignOutboundCall(const shared_ptr<OutboundCall>&
call) {
                                    call->controller()->credentials_policy(),
                                    &conn);
   if (PREDICT_FALSE(!s.ok())) {
-    call->SetFailed(s, OutboundCall::Phase::CONNECTION_NEGOTIATION);
+    call->SetFailed(std::move(s), OutboundCall::Phase::CONNECTION_NEGOTIATION);
     return;
   }
 


Mime
View raw message