You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/12/01 01:15:12 UTC
[9/9] kudu git commit: KUDU-1775 (part 1). Reject CREATE TABLE with
negative or too-high replication factors
KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
This adds more master-side checking of the replication factor passed
from the client:
* must be non-negative. Previously passing a negative replication factor
would result in infinite retries trying to assign replicas.
* put an upper bound of 7, configurable with a new unsafe flag. We
haven't tested higher replication factors at all, and a
well-intentioned user may try to set replication factor to a very high
number to get a table replicated onto every server in their cluster.
This is common practice in MPP RDBMS, but since we haven't tested it,
we shouldn't allow it without making the user turn on
--unlock_unsafe_flags.
Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Reviewed-on: http://gerrit.cloudera.org:8080/5289
Reviewed-by: Adar Dembo <ad...@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/b39491de
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b39491de
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b39491de
Branch: refs/heads/master
Commit: b39491de611eb7a36be46816c66fa3a85590a656
Parents: e4f7e92
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Nov 30 14:23:20 2016 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Thu Dec 1 01:14:03 2016 +0000
----------------------------------------------------------------------
src/kudu/client/client-test.cc | 50 +++++++++++---------------
src/kudu/client/client.cc | 5 +--
src/kudu/client/table_creator-internal.cc | 1 -
src/kudu/client/table_creator-internal.h | 3 +-
src/kudu/master/catalog_manager.cc | 23 +++++++++++-
src/kudu/master/master.proto | 5 ++-
6 files changed, 52 insertions(+), 35 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/client/client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index c52cd76..77aa1a1 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -3760,24 +3760,27 @@ TEST_F(ClientTest, TestCreateTableWithTooManyTablets) {
"The requested number of tablets is over the permitted maximum (1)");
}
-TEST_F(ClientTest, TestCreateTableWithTooManyReplicas) {
- KuduPartialRow* split1 = schema_.NewRow();
- ASSERT_OK(split1->SetInt32("key", 1));
-
- KuduPartialRow* split2 = schema_.NewRow();
- ASSERT_OK(split2->SetInt32("key", 2));
+// Tests for too many replicas, too few replicas, even replica count, etc.
+TEST_F(ClientTest, TestCreateTableWithBadNumReplicas) {
+ const vector<pair<int, string>> cases = {
+ {3, "Not enough live tablet servers to create a table with the requested "
+ "replication factor 3. 1 tablet servers are alive"},
+ {2, "illegal replication factor 2 (replication factor must be odd)"},
+ {-1, "illegal replication factor -1 (replication factor must be positive)"},
+ {11, "illegal replication factor 11 (max replication factor is 7)"}
+ };
- gscoped_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
- Status s = table_creator->table_name("foobar")
- .schema(&schema_)
- .set_range_partition_columns({ "key" })
- .split_rows({ split1, split2 })
- .num_replicas(3)
- .Create();
- ASSERT_TRUE(s.IsInvalidArgument());
- ASSERT_STR_CONTAINS(s.ToString(),
- "Not enough live tablet servers to create a table with the requested "
- "replication factor 3. 1 tablet servers are alive");
+ for (const auto& c : cases) {
+ SCOPED_TRACE(Substitute("num_replicas=$0", c.first));
+ gscoped_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
+ Status s = table_creator->table_name("foobar")
+ .schema(&schema_)
+ .set_range_partition_columns({ "key" })
+ .num_replicas(c.first)
+ .Create();
+ EXPECT_TRUE(s.IsInvalidArgument());
+ ASSERT_STR_CONTAINS(s.ToString(), c.second);
+ }
}
TEST_F(ClientTest, TestCreateTableWithInvalidEncodings) {
@@ -4182,7 +4185,7 @@ TEST_F(ClientTest, TestBatchScanConstIterator) {
}
TEST_F(ClientTest, TestTableNumReplicas) {
- for (int i : { 1, 3, 5, 7, 9 }) {
+ for (int i : { 1, 3, 5, 7 }) {
shared_ptr<KuduTable> table;
NO_FATALS(CreateTable(Substitute("table_with_$0_replicas", i),
i, {}, {}, &table));
@@ -4227,16 +4230,5 @@ TEST_F(ClientTest, TestGetTablet) {
}
}
-// Test create table with even replicas factor should fail.
-TEST_F(ClientTest, TestTableWithEvenReplicas) {
- gscoped_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
- Status s = table_creator->table_name("table_with_even_replicas")
- .schema(&schema_)
- .num_replicas(2)
- .set_range_partition_columns({ "key" })
- .Create();
- ASSERT_TRUE(s.IsInvalidArgument());
- ASSERT_STR_CONTAINS(s.ToString(), "Illegal replication factor");
-}
} // namespace client
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/client/client.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index f700efc..974b4be 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -26,6 +26,7 @@
#include <vector>
#include <boost/bind.hpp>
+#include <boost/optional.hpp>
#include "kudu/client/batcher.h"
#include "kudu/client/callbacks.h"
@@ -588,8 +589,8 @@ Status KuduTableCreator::Create() {
// Build request.
CreateTableRequestPB req;
req.set_name(data_->table_name_);
- if (data_->num_replicas_ >= 1) {
- req.set_num_replicas(data_->num_replicas_);
+ if (data_->num_replicas_ != boost::none) {
+ req.set_num_replicas(data_->num_replicas_.get());
}
RETURN_NOT_OK_PREPEND(SchemaToPB(*data_->schema_->schema_, req.mutable_schema(),
SCHEMA_PB_WITHOUT_WRITE_DEFAULT),
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/client/table_creator-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/table_creator-internal.cc b/src/kudu/client/table_creator-internal.cc
index 36311a9..118919c 100644
--- a/src/kudu/client/table_creator-internal.cc
+++ b/src/kudu/client/table_creator-internal.cc
@@ -27,7 +27,6 @@ namespace client {
KuduTableCreator::Data::Data(KuduClient* client)
: client_(client),
schema_(nullptr),
- num_replicas_(0),
wait_(true) {
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/client/table_creator-internal.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/table_creator-internal.h b/src/kudu/client/table_creator-internal.h
index 7a6753d..57f46dc 100644
--- a/src/kudu/client/table_creator-internal.h
+++ b/src/kudu/client/table_creator-internal.h
@@ -17,6 +17,7 @@
#ifndef KUDU_CLIENT_TABLE_CREATOR_INTERNAL_H
#define KUDU_CLIENT_TABLE_CREATOR_INTERNAL_H
+#include <boost/optional.hpp>
#include <memory>
#include <string>
#include <utility>
@@ -53,7 +54,7 @@ class KuduTableCreator::Data {
PartitionSchemaPB partition_schema_;
- int num_replicas_;
+ boost::optional<int> num_replicas_;
MonoDelta timeout_;
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 67be524..d5af69c 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -116,6 +116,11 @@ DEFINE_int32(default_num_replicas, 3,
"Default number of replicas for tables that do not have the num_replicas set.");
TAG_FLAG(default_num_replicas, advanced);
+DEFINE_int32(max_num_replicas, 7,
+ "Maximum number of replicas that may be specified for a table.");
+// Tag as unsafe since we have done very limited testing of higher than 5 replicas.
+TAG_FLAG(max_num_replicas, unsafe);
+
DEFINE_bool(allow_unsafe_replication_factor, false,
"Allow creating tables with even replication factor.");
TAG_FLAG(allow_unsafe_replication_factor, unsafe);
@@ -933,11 +938,27 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
// Reject create table with even replication factors, unless master flag
// allow_unsafe_replication_factor is on.
if (req.num_replicas() % 2 == 0 && !FLAGS_allow_unsafe_replication_factor) {
- s = Status::InvalidArgument(Substitute("Illegal replication factor $0 (replication "
+ s = Status::InvalidArgument(Substitute("illegal replication factor $0 (replication "
"factor must be odd)", req.num_replicas()));
return SetError(MasterErrorPB::EVEN_REPLICATION_FACTOR, s);
}
+ if (req.num_replicas() > FLAGS_max_num_replicas) {
+ s = Status::InvalidArgument(Substitute("illegal replication factor $0 (max replication "
+ "factor is $1)",
+ req.num_replicas(),
+ FLAGS_max_num_replicas));
+ return SetError(MasterErrorPB::REPLICATION_FACTOR_TOO_HIGH, s);
+
+ }
+ if (req.num_replicas() <= 0) {
+ s = Status::InvalidArgument(Substitute("illegal replication factor $0 (replication factor "
+ "must be positive)",
+ req.num_replicas(),
+ FLAGS_max_num_replicas));
+ return SetError(MasterErrorPB::ILLEGAL_REPLICATION_FACTOR, s);
+ }
+
// Verify that the total number of tablets is reasonable, relative to the number
// of live tablet servers.
TSDescriptorVector ts_descs;
http://git-wip-us.apache.org/repos/asf/kudu/blob/b39491de/src/kudu/master/master.proto
----------------------------------------------------------------------
diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto
index 17a263d..fb02dc1 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -58,7 +58,7 @@ message MasterErrorPB {
NOT_THE_LEADER = 7;
// The number of replicas requested is greater than the number of live servers
- // in the cluster.
+ // in the cluster or the configured maximum.
REPLICATION_FACTOR_TOO_HIGH = 8;
// The request or response involved a tablet which is not yet running.
@@ -66,6 +66,9 @@ message MasterErrorPB {
// The number of replicas requested is even.
EVEN_REPLICATION_FACTOR = 10;
+
+ // The number of replicas requested is illegal (eg non-positive).
+ ILLEGAL_REPLICATION_FACTOR = 11;
}
// The error code.