This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new b2fae6d [catalog_manager] Status::AlreadyPresent for range duplicates
b2fae6d is described below
commit b2fae6d0aec813c9a82c4cd1cbb3e5d495d6e35a
Author: Alexey Serbin <[email protected]>
AuthorDate: Fri Oct 2 22:54:54 2020 -0700
[catalog_manager] Status::AlreadyPresent for range duplicates
With this patch, CatalogManager discriminates between overlapped
and exact duplicate key ranges when adding new partitions, returning
Status::AlreadyPresent() for exact range duplicates and
Status::InvalidArgument for otherwise overlapped ones. The number
of hash buckets is not taken into account when searching/reporting
about the exact duplicates in key range for partitions.
Before this patch, CatalogManager returned Status::InvalidArgument()
for exact duplicates and otherwise overlapped ranges as well.
This patch also updates the relevant tests, so the new behavior
has enough test coverage.
A follow-up patch will use the newly introduced finer status reporting.
To be more precise, TxnManager could send multiple requests to add the
same new tablet for the transaction status table while processing
concurrent BeginTransaction() requests. With that, it should be able
to tell between an error when the corresponding tablet is already
present and other types of errors.
Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Reviewed-on: http://gerrit.cloudera.org:8080/16544
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <[email protected]>
---
.../org/apache/kudu/client/TestAlterTable.java | 18 +--
src/kudu/integration-tests/alter_table-test.cc | 125 ++++++++++++---------
.../integration-tests/txn_status_table-itest.cc | 2 +-
src/kudu/master/catalog_manager.cc | 105 +++++++++++------
4 files changed, 148 insertions(+), 102 deletions(-)
diff --git
a/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
b/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
index 6f05a25..5d88178 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
@@ -389,7 +389,7 @@ public class TestAlterTable {
insertRows(table, 0, 100);
assertEquals(100, countRowsInTable(table));
- // ADD [0, 100) <- illegal (duplicate)
+ // ADD [0, 100) <- already present (duplicate)
PartialRow lower = schema.newPartialRow();
PartialRow upper = schema.newPartialRow();
lower.addInt("c0", 0);
@@ -398,9 +398,9 @@ public class TestAlterTable {
client.alterTable(tableName, new
AlterTableOptions().addRangePartition(lower, upper));
fail();
} catch (KuduException e) {
- assertTrue(e.getStatus().isInvalidArgument());
+ assertTrue(e.getStatus().isAlreadyPresent());
assertTrue(e.getStatus().getMessage().contains(
- "New range partition conflicts with existing range partition"));
+ "range partition already exists"));
}
assertEquals(100, countRowsInTable(table));
@@ -413,7 +413,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage().contains(
- "New range partition conflicts with existing range partition"));
+ "new range partition conflicts with existing one"));
}
assertEquals(100, countRowsInTable(table));
@@ -426,7 +426,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage().contains(
- "New range partition conflicts with existing range partition"));
+ "new range partition conflicts with existing one"));
}
assertEquals(100, countRowsInTable(table));
@@ -445,7 +445,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage().contains(
- "New range partition conflicts with existing range partition"));
+ "new range partition conflicts with existing one"));
}
assertEquals(100, countRowsInTable(table));
@@ -458,7 +458,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage(),
e.getStatus().getMessage().contains(
- "No range partition found for drop range partition step"));
+ "no range partition to drop"));
}
assertEquals(100, countRowsInTable(table));
@@ -473,7 +473,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage().contains(
- "No range partition found for drop range partition step"));
+ "no range partition to drop"));
}
assertEquals(100, countRowsInTable(table));
assertFalse(client.tableExists("foo"));
@@ -507,7 +507,7 @@ public class TestAlterTable {
} catch (KuduException e) {
assertTrue(e.getStatus().isInvalidArgument());
assertTrue(e.getStatus().getMessage().contains(
- "No range partition found for drop range partition step"));
+ "no range partition to drop"));
}
assertEquals(100, countRowsInTable(table));
}
diff --git a/src/kudu/integration-tests/alter_table-test.cc
b/src/kudu/integration-tests/alter_table-test.cc
index d9b62d4..69dc64e 100644
--- a/src/kudu/integration-tests/alter_table-test.cc
+++ b/src/kudu/integration-tests/alter_table-test.cc
@@ -1417,7 +1417,7 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(InsertRowsSequential(table_name, 0, 100));
ASSERT_EQ(100, CountTableRows(table.get()));
- // ADD [0, 100) <- illegal (duplicate)
+ // ADD [0, 100) <- already present (duplicate)
table_alterer.reset(client_->NewTableAlterer(table_name));
lower.reset(schema_.NewRow());
upper.reset(schema_.NewRow());
@@ -1425,8 +1425,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 100));
table_alterer->AddRangePartition(lower.release(), upper.release());
s = table_alterer->wait(false)->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "New range partition conflicts with
existing range partition");
+ ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "range partition already exists");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [50, 150) <- illegal (overlap)
@@ -1437,8 +1437,9 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 150));
table_alterer->AddRangePartition(lower.release(), upper.release());
s = table_alterer->wait(false)->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "New range partition conflicts with
existing range partition");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "new range partition conflicts with existing one");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD (-50, 50] <- illegal (overlap)
@@ -1451,8 +1452,9 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
KuduTableCreator::EXCLUSIVE_BOUND,
KuduTableCreator::INCLUSIVE_BOUND);
s = table_alterer->wait(false)->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "New range partition conflicts with
existing range partition");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "new range partition conflicts with existing one");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [200, 300)
@@ -1469,8 +1471,9 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 150));
table_alterer->AddRangePartition(lower.release(), upper.release());
s = table_alterer->wait(false)->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "New range partition conflicts with
existing range partition");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "new range partition conflicts with existing one");
ASSERT_FALSE(InsertRowsSequential(table_name, 200, 100).ok());
ASSERT_EQ(100, CountTableRows(table.get()));
@@ -1478,8 +1481,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
table_alterer.reset(client_->NewTableAlterer(table_name));
table_alterer->DropRangePartition(schema_.NewRow(), schema_.NewRow());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [50, 150)
@@ -1492,8 +1495,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
table_alterer->DropRangePartition(lower.release(), upper.release());
table_alterer->RenameTo("foo");
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [-50, 50)
@@ -1504,8 +1507,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [0, 100)
@@ -1540,8 +1543,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 10));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->wait(false)->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// KUDU-1750 Regression cases:
@@ -1554,8 +1557,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [50, 100) <- illegal
@@ -1566,8 +1569,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 100));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [100, 200)
@@ -1584,8 +1587,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 150));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [100, 200)
@@ -1602,8 +1605,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 200));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [<min>, 0)
@@ -1618,8 +1621,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", -50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [<min>, 0)
@@ -1634,8 +1637,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [100, <max>)
@@ -1650,8 +1653,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(lower->SetInt32("c0", 150));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// ADD [100, <max>)
@@ -1666,8 +1669,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(lower->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// Setup for the next few test cases:
@@ -1691,8 +1694,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", -50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [<min>, 50] <- illegal
@@ -1702,8 +1705,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(upper->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [150, <max>] <- illegal
@@ -1713,8 +1716,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(lower->SetInt32("c0", 150));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [50, <max>] <- illegal
@@ -1724,8 +1727,8 @@ TEST_F(AlterTableTest, TestAlterRangePartitioningInvalid)
{
ASSERT_OK(lower->SetInt32("c0", 50));
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// DROP [<min>, <max>] <- illegal
@@ -1734,15 +1737,15 @@ TEST_F(AlterTableTest,
TestAlterRangePartitioningInvalid) {
upper.reset(schema_.NewRow());
table_alterer->DropRangePartition(lower.release(), upper.release());
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop range
partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
ASSERT_EQ(100, CountTableRows(table.get()));
// Bad arguments (null ranges)
table_alterer.reset(client_->NewTableAlterer(table_name));
table_alterer->DropRangePartition(nullptr, nullptr);
s = table_alterer->Alter();
- ASSERT_FALSE(s.ok());
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
ASSERT_STR_CONTAINS(s.ToString(), "range partition bounds may not be null");
}
@@ -1856,19 +1859,30 @@ TEST_F(AlterTableTest,
TestAddRangePartitionConflictExhaustive) {
// Add a then add b.
ASSERT_OK(add_range_partition(a_lower_bound, a_upper_bound));
- Status s = add_range_partition(b_lower_bound, b_upper_bound);
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(),
- "New range partition conflicts with existing range
partition");
+ auto s = add_range_partition(b_lower_bound, b_upper_bound);
+ if (a_lower_bound == b_lower_bound && a_upper_bound == b_upper_bound) {
+ ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "range partition already exists");
+ } else {
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "new range partition conflicts with existing one");
+ }
// Clean up by removing a.
ASSERT_OK(drop_range_partition(a_lower_bound, a_upper_bound));
// Add a and b in the same op.
s = add_range_partitions(a_lower_bound, a_upper_bound,
b_lower_bound, b_upper_bound);
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(),
- "New range partition conflicts with another new range
partition");
+ if (a_lower_bound == b_lower_bound && a_upper_bound == b_upper_bound) {
+ ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "new range partiton duplicates another newly added
one");
+ } else {
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(),
+ "new range partition conflicts with another newly
added one");
+ }
// To get some extra coverage of DROP RANGE PARTITION, check if the two
// ranges are not equal, and if so, check that adding one and dropping the
@@ -1878,17 +1892,16 @@ TEST_F(AlterTableTest,
TestAddRangePartitionConflictExhaustive) {
// Add a then drop b.
ASSERT_OK(add_range_partition(a_lower_bound, a_upper_bound));
Status s = drop_range_partition(b_lower_bound, b_upper_bound);
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(), "No range partition found for drop
range partition step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
// Clean up by removing a.
ASSERT_OK(drop_range_partition(a_lower_bound, a_upper_bound));
// Add a and drop b in a single op.
s = add_drop_range_partitions(a_lower_bound, a_upper_bound,
b_lower_bound, b_upper_bound);
- ASSERT_FALSE(s.ok());
- ASSERT_STR_CONTAINS(s.ToString(),
- "No range partition found for drop range partition
step");
+ ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "no range partition to drop");
}
};
diff --git a/src/kudu/integration-tests/txn_status_table-itest.cc
b/src/kudu/integration-tests/txn_status_table-itest.cc
index 3d85b7e..7674d55 100644
--- a/src/kudu/integration-tests/txn_status_table-itest.cc
+++ b/src/kudu/integration-tests/txn_status_table-itest.cc
@@ -326,7 +326,7 @@ TEST_F(TxnStatusTableITest,
TestCreateTxnStatusTablePartitions) {
// Now add more partitions and try again.
ASSERT_OK(txn_sys_client_->AddTxnStatusTableRange(100, 200));
s = txn_sys_client_->AddTxnStatusTableRange(100, 200);
- ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
+ ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
NO_FATALS(CheckTableTypes({ { TableTypePB::TXN_STATUS_TABLE, 2 } }));
// Ensure we still create transaction status tablets even after the master is
diff --git a/src/kudu/master/catalog_manager.cc
b/src/kudu/master/catalog_manager.cc
index 5438e41..9475fa4 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2431,8 +2431,9 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
}
if (ops.size() != 2) {
- return Status::InvalidArgument("expected two row operations for alter
range partition step",
- SecureShortDebugString(step));
+ return Status::InvalidArgument(
+ "expected two row operations for alter range partition step",
+ SecureShortDebugString(step));
}
if ((ops[0].type != RowOperationsPB::RANGE_LOWER_BOUND &&
@@ -2441,7 +2442,7 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
ops[1].type != RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND)) {
return Status::InvalidArgument(
"expected a lower bound and upper bound row op for alter range
partition step",
- strings::Substitute("$0, $1", ops[0].ToString(schema),
ops[1].ToString(schema)));
+ Substitute("$0, $1", ops[0].ToString(schema),
ops[1].ToString(schema)));
}
if (ops[0].type == RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND) {
@@ -2454,65 +2455,97 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
}
vector<Partition> partitions;
- RETURN_NOT_OK(partition_schema.CreatePartitions({}, {{ *ops[0].split_row,
*ops[1].split_row }},
- schema, &partitions));
-
+ RETURN_NOT_OK(partition_schema.CreatePartitions(
+ {}, {{ *ops[0].split_row, *ops[1].split_row }}, schema, &partitions));
switch (step.type()) {
case AlterTableRequestPB::ADD_RANGE_PARTITION: {
for (const Partition& partition : partitions) {
const string& lower_bound = partition.partition_key_start();
const string& upper_bound = partition.partition_key_end();
- // Check that the new tablet doesn't overlap with the existing
tablets.
- // Iter points at the tablet directly *after* the lower bound (or to
- // existing_tablets.end(), if such a tablet does not exist).
- auto existing_iter = existing_tablets.upper_bound(lower_bound);
+ // Check that the new tablet does not overlap with any of the
existing
+ // tablets. Since the elements of 'existing_tablets' are ordered by
+ // the tablets' lower bounds, the iterator points at the tablet
+ // directly *after* the lower bound or to existing_tablets.end()
+ // if such a tablet does not exist.
+ const auto existing_iter = existing_tablets.upper_bound(lower_bound);
if (existing_iter != existing_tablets.end()) {
- TabletMetadataLock metadata(existing_iter->second.get(),
LockMode::READ);
- if (upper_bound.empty() ||
- metadata.data().pb.partition().partition_key_start() <
upper_bound) {
+ TabletMetadataLock metadata(existing_iter->second.get(),
+ LockMode::READ);
+ const auto& p = metadata.data().pb.partition();
+ // Check for the overlapping ranges.
+ if (upper_bound.empty() || p.partition_key_start() < upper_bound) {
return Status::InvalidArgument(
- "New range partition conflicts with existing range
partition",
-
partition_schema.RangePartitionDebugString(*ops[0].split_row,
*ops[1].split_row));
+ "new range partition conflicts with existing one",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
}
}
+ // This is the case when there is an existing tablet with the lower
+ // bound being less or equal to the lower bound of the new tablet to
+ // create. This cannot be the case of an empty 'existing_tablets'
+ // container (otherwise, existing_tablets.end() would be equal to
+ // existing_tablets.begin()), so it's safe to decrement the iterator
+ // (i.e. call std::prev() on it) and de-reference it.
if (existing_iter != existing_tablets.begin()) {
TabletMetadataLock metadata(std::prev(existing_iter)->second.get(),
LockMode::READ);
- if (metadata.data().pb.partition().partition_key_end().empty() ||
- metadata.data().pb.partition().partition_key_end() >
lower_bound) {
+ const auto& p = metadata.data().pb.partition();
+ // Check for the exact match of ranges.
+ if (lower_bound == p.partition_key_start() &&
+ upper_bound == p.partition_key_end()) {
+ return Status::AlreadyPresent(
+ "range partition already exists",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
+ }
+ // Check for the overlapping ranges.
+ if (p.partition_key_end().empty() ||
+ p.partition_key_end() > lower_bound) {
return Status::InvalidArgument(
- "New range partition conflicts with existing range
partition",
-
partition_schema.RangePartitionDebugString(*ops[0].split_row,
*ops[1].split_row));
+ "new range partition conflicts with existing one",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
}
}
// Check that the new tablet doesn't overlap with any other new
tablets.
auto new_iter = new_tablets.upper_bound(lower_bound);
if (new_iter != new_tablets.end()) {
- const auto& metadata =
new_iter->second->mutable_metadata()->dirty();
- if (upper_bound.empty() ||
- metadata.pb.partition().partition_key_start() < upper_bound) {
+ // Check for the overlapping ranges.
+ const auto& p =
new_iter->second->mutable_metadata()->dirty().pb.partition();
+ if (upper_bound.empty() || p.partition_key_start() < upper_bound) {
return Status::InvalidArgument(
- "New range partition conflicts with another new range
partition",
-
partition_schema.RangePartitionDebugString(*ops[0].split_row,
*ops[1].split_row));
+ "new range partition conflicts with another newly added one",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
}
}
if (new_iter != new_tablets.begin()) {
- const auto& metadata =
std::prev(new_iter)->second->mutable_metadata()->dirty();
- if (metadata.pb.partition().partition_key_end().empty() ||
- metadata.pb.partition().partition_key_end() > lower_bound) {
+ const auto& p =
std::prev(new_iter)->second->mutable_metadata()->dirty().pb.partition();
+ // Check for the exact match of ranges.
+ if (lower_bound == p.partition_key_start() &&
+ upper_bound == p.partition_key_end()) {
+ return Status::AlreadyPresent(
+ "new range partiton duplicates another newly added one",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
+ }
+ // Check for the overlapping ranges.
+ if (p.partition_key_end().empty() || p.partition_key_end() >
lower_bound) {
return Status::InvalidArgument(
- "New range partition conflicts with another new range
partition",
-
partition_schema.RangePartitionDebugString(*ops[0].split_row,
*ops[1].split_row));
+ "new range partition conflicts with another newly added one",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+
*ops[1].split_row));
}
}
- PartitionPB partition_pb;
- partition.ToPB(&partition_pb);
- const optional<string> dimension_label =
step.add_range_partition().has_dimension_label()
+ const optional<string> dimension_label =
+ step.add_range_partition().has_dimension_label()
? make_optional(step.add_range_partition().dimension_label())
: none;
+ PartitionPB partition_pb;
+ partition.ToPB(&partition_pb);
new_tablets.emplace(lower_bound,
CreateTabletInfo(table, partition_pb,
dimension_label));
}
@@ -2551,15 +2584,15 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
new_iter->second->mutable_metadata()->AbortMutation();
new_tablets.erase(new_iter);
} else {
- return Status::InvalidArgument(
- "No range partition found for drop range partition step",
- partition_schema.RangePartitionDebugString(*ops[0].split_row,
*ops[1].split_row));
+ return Status::InvalidArgument("no range partition to drop",
+ partition_schema.RangePartitionDebugString(*ops[0].split_row,
+ *ops[1].split_row));
}
}
break;
}
default: {
- return Status::InvalidArgument("Unknown alter table range partitioning
step",
+ return Status::InvalidArgument("unknown alter table range partitioning
step",
SecureShortDebugString(step));
}
}
