You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2017/10/17 01:18:24 UTC
kudu git commit: Miscellaneous cleanup
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 <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@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 <da...@apache.org>
Authored: Thu Oct 12 18:07:11 2017 -0700
Committer: Dan Burkert <da...@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;
}