Repository: kudu Updated Branches: refs/heads/master 2b5ad4a9b -> c553a9a59
KUDU-1658: Reject CREATE TABLE ops with even replication factor Reject table creation with even replication factor, and add an allow_unsafe_replication_factor master flag to make it possible for advanced user. Change-Id: I1674cc59cdfc2955d42fc5e4d8c0d962d9cc8e8e Reviewed-on: http://gerrit.cloudera.org:8080/4945 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c553a9a5 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c553a9a5 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c553a9a5 Branch: refs/heads/master Commit: c553a9a59ce664c38041fbb3809ab6402cdbc89b Parents: 2b5ad4a Author: hahao <[email protected]> Authored: Tue Nov 8 18:25:40 2016 -0800 Committer: Todd Lipcon <[email protected]> Committed: Wed Nov 30 18:38:54 2016 +0000 ---------------------------------------------------------------------- src/kudu/client/client-test.cc | 15 ++++++++++++ src/kudu/integration-tests/delete_table-test.cc | 8 +++++-- .../integration-tests/raft_consensus-itest.cc | 24 ++++++++++++-------- src/kudu/integration-tests/tablet_copy-itest.cc | 8 ++++--- .../ts_tablet_manager-itest.cc | 4 ++++ src/kudu/master/catalog_manager.cc | 12 ++++++++++ src/kudu/master/master.proto | 3 +++ src/kudu/tools/kudu-admin-test.cc | 3 ++- 8 files changed, 62 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/client/client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index 58c48c6..c52cd76 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -72,6 +72,7 @@ DECLARE_bool(enable_data_block_fsync); DECLARE_bool(fail_dns_resolution); DECLARE_bool(log_inject_latency); +DECLARE_bool(allow_unsafe_replication_factor); DECLARE_int32(heartbeat_interval_ms); DECLARE_int32(log_inject_latency_ms_mean); DECLARE_int32(log_inject_latency_ms_stddev); @@ -1144,6 +1145,9 @@ TEST_F(ClientTest, TestScanFaultTolerance) { const string kScanTable = "TestScanFaultTolerance"; shared_ptr<KuduTable> table; + // Allow creating table with even replication factor. + FLAGS_allow_unsafe_replication_factor = true; + // We use only two replicas in this test so that every write is fully replicated to both // servers (the Raft majority is 2/2). This reduces potential flakiness if the scanner tries // to read from a replica that is lagging for some reason. This won't be necessary once @@ -4223,5 +4227,16 @@ 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/c553a9a5/src/kudu/integration-tests/delete_table-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/delete_table-test.cc b/src/kudu/integration-tests/delete_table-test.cc index 0665d0f..08b48c3 100644 --- a/src/kudu/integration-tests/delete_table-test.cc +++ b/src/kudu/integration-tests/delete_table-test.cc @@ -397,7 +397,10 @@ TEST_F(DeleteTableTest, TestAutoTombstoneAfterCrashDuringTabletCopy) { "--flush_threshold_mb=0", "--maintenance_manager_polling_interval_ms=100" }; - NO_FATALS(StartCluster(ts_flags)); + vector<string> master_flags = { + "--allow_unsafe_replication_factor=true" + }; + NO_FATALS(StartCluster(ts_flags, master_flags)); const MonoDelta timeout = MonoDelta::FromSeconds(10); const int kTsIndex = 0; // We'll test with the first TS. @@ -553,7 +556,8 @@ TEST_F(DeleteTableTest, TestAutoTombstoneAfterTabletCopyRemoteFails) { "--log_segment_size_mb=1" // Faster log rolls. }; vector<string> master_flags = { - "--catalog_manager_wait_for_new_tablets_to_elect_leader=false" + "--catalog_manager_wait_for_new_tablets_to_elect_leader=false", + "--allow_unsafe_replication_factor=true" }; NO_FATALS(StartCluster(ts_flags, master_flags)); const MonoDelta kTimeout = MonoDelta::FromSeconds(20); http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/integration-tests/raft_consensus-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/raft_consensus-itest.cc b/src/kudu/integration-tests/raft_consensus-itest.cc index 41dedb0..6582626 100644 --- a/src/kudu/integration-tests/raft_consensus-itest.cc +++ b/src/kudu/integration-tests/raft_consensus-itest.cc @@ -1982,7 +1982,8 @@ TEST_F(RaftConsensusITest, TestMasterNotifiedOnConfigChange) { FLAGS_num_tablet_servers = 3; FLAGS_num_replicas = 2; vector<string> ts_flags; - vector<string> master_flags = { "--master_add_server_when_underreplicated=false" }; + vector<string> master_flags = { "--master_add_server_when_underreplicated=false", + "--allow_unsafe_replication_factor=true"}; NO_FATALS(BuildAndStart(ts_flags, master_flags)); LOG(INFO) << "Finding tablet leader and waiting for things to start..."; @@ -2133,14 +2134,19 @@ TEST_F(RaftConsensusITest, TestEarlyCommitDespiteMemoryPressure) { TEST_F(RaftConsensusITest, TestAutoCreateReplica) { FLAGS_num_tablet_servers = 3; FLAGS_num_replicas = 2; - vector<string> ts_flags, master_flags; - ts_flags.push_back("--enable_leader_failure_detection=false"); - ts_flags.push_back("--log_cache_size_limit_mb=1"); - ts_flags.push_back("--log_segment_size_mb=1"); - ts_flags.push_back("--log_async_preallocate_segments=false"); - ts_flags.push_back("--flush_threshold_mb=1"); - ts_flags.push_back("--maintenance_manager_polling_interval_ms=300"); - master_flags.push_back("--catalog_manager_wait_for_new_tablets_to_elect_leader=false"); + + vector<string> ts_flags = { + "--enable_leader_failure_detection=false", + "--log_cache_size_limit_mb=1", + "--log_segment_size_mb=1", + "--log_async_preallocate_segments=false", + "--flush_threshold_mb=1", + "--maintenance_manager_polling_interval_ms=300", + }; + vector<string> master_flags = { + "--catalog_manager_wait_for_new_tablets_to_elect_leader=false", + "--allow_unsafe_replication_factor=true" + }; BuildAndStart(ts_flags, master_flags); // 50K is enough to cause flushes & log rolls. http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/integration-tests/tablet_copy-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/tablet_copy-itest.cc b/src/kudu/integration-tests/tablet_copy-itest.cc index b201d0a..071f18c 100644 --- a/src/kudu/integration-tests/tablet_copy-itest.cc +++ b/src/kudu/integration-tests/tablet_copy-itest.cc @@ -409,9 +409,11 @@ TEST_F(TabletCopyITest, TestDeleteTabletDuringTabletCopy) { // as the tablet copy source. When a tablet is tombstoned, its last-logged // opid is stored in a field its on-disk superblock. TEST_F(TabletCopyITest, TestTabletCopyFollowerWithHigherTerm) { - vector<string> ts_flags, master_flags; - ts_flags.push_back("--enable_leader_failure_detection=false"); - master_flags.push_back("--catalog_manager_wait_for_new_tablets_to_elect_leader=false"); + vector<string> ts_flags = { "--enable_leader_failure_detection=false" }; + vector<string> master_flags = { + "--catalog_manager_wait_for_new_tablets_to_elect_leader=false", + "--allow_unsafe_replication_factor=true" + }; const int kNumTabletServers = 2; NO_FATALS(StartCluster(ts_flags, master_flags, kNumTabletServers)); http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/integration-tests/ts_tablet_manager-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/ts_tablet_manager-itest.cc b/src/kudu/integration-tests/ts_tablet_manager-itest.cc index 3ada5ce..a963eb9 100644 --- a/src/kudu/integration-tests/ts_tablet_manager-itest.cc +++ b/src/kudu/integration-tests/ts_tablet_manager-itest.cc @@ -44,6 +44,7 @@ DECLARE_bool(enable_leader_failure_detection); DECLARE_bool(catalog_manager_wait_for_new_tablets_to_elect_leader); +DECLARE_bool(allow_unsafe_replication_factor); DEFINE_int32(num_election_test_loops, 3, "Number of random EmulateElection() loops to execute in " "TestReportNewLeaderOnLeaderChange"); @@ -107,6 +108,9 @@ TEST_F(TsTabletManagerITest, TestReportNewLeaderOnLeaderChange) { FLAGS_enable_leader_failure_detection = false; FLAGS_catalog_manager_wait_for_new_tablets_to_elect_leader = false; + // Allow creating table with even replication factor. + FLAGS_allow_unsafe_replication_factor = true; + // Run a few more iters in slow-test mode. OverrideFlagForSlowTests("num_election_test_loops", "10"); http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 472316c..67be524 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -116,6 +116,10 @@ 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_bool(allow_unsafe_replication_factor, false, + "Allow creating tables with even replication factor."); +TAG_FLAG(allow_unsafe_replication_factor, unsafe); + DEFINE_int32(catalog_manager_bg_task_wait_ms, 1000, "Amount of time the catalog manager background task thread waits " "between runs"); @@ -926,6 +930,14 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, req.set_num_replicas(FLAGS_default_num_replicas); } + // 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 " + "factor must be odd)", req.num_replicas())); + return SetError(MasterErrorPB::EVEN_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/c553a9a5/src/kudu/master/master.proto ---------------------------------------------------------------------- diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index 7b013c4..17a263d 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -63,6 +63,9 @@ message MasterErrorPB { // The request or response involved a tablet which is not yet running. TABLET_NOT_RUNNING = 9; + + // The number of replicas requested is even. + EVEN_REPLICATION_FACTOR = 10; } // The error code. http://git-wip-us.apache.org/repos/asf/kudu/blob/c553a9a5/src/kudu/tools/kudu-admin-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc index 0272550..1a34718 100644 --- a/src/kudu/tools/kudu-admin-test.cc +++ b/src/kudu/tools/kudu-admin-test.cc @@ -57,7 +57,8 @@ TEST_F(AdminCliTest, TestChangeConfig) { FLAGS_num_tablet_servers = 3; FLAGS_num_replicas = 2; BuildAndStart({ "--enable_leader_failure_detection=false" }, - { "--catalog_manager_wait_for_new_tablets_to_elect_leader=false" }); + { "--catalog_manager_wait_for_new_tablets_to_elect_leader=false", + "--allow_unsafe_replication_factor=true"}); vector<TServerDetails*> tservers; AppendValuesFromMap(tablet_servers_, &tservers);
