You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by la...@apache.org on 2021/11/02 06:22:38 UTC
[kudu] branch master updated: KUDU-3332 [tools] Fix too large
next_column_id after unsafe_rebuilding master
This is an automated email from the ASF dual-hosted git repository.
laiyingchun 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 a31b96e KUDU-3332 [tools] Fix too large next_column_id after unsafe_rebuilding master
a31b96e is described below
commit a31b96edee11c08c909a717b9195c42b5e678a47
Author: Yingchun Lai <ac...@gmail.com>
AuthorDate: Thu Oct 28 23:47:21 2021 +0800
KUDU-3332 [tools] Fix too large next_column_id after unsafe_rebuilding master
Change-Id: I5ccacc2e028a7c278a912bb68620aefec44026b9
Reviewed-on: http://gerrit.cloudera.org:8080/17984
Tested-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/common/id_mapping.h | 8 ++---
src/kudu/tools/kudu-admin-test.cc | 61 ++++++++++++++++++++++++++++++++++++--
src/kudu/tools/master_rebuilder.cc | 28 ++++++++++-------
src/kudu/tools/master_rebuilder.h | 2 ++
4 files changed, 82 insertions(+), 17 deletions(-)
diff --git a/src/kudu/common/id_mapping.h b/src/kudu/common/id_mapping.h
index caec450..8d17f66 100644
--- a/src/kudu/common/id_mapping.h
+++ b/src/kudu/common/id_mapping.h
@@ -70,10 +70,8 @@ class IdMapping {
// NOLINT on this function definition because it thinks we're calling
// std::swap instead of defining it.
void swap(IdMapping& other) { // NOLINT(*)
- uint64_t tmp = other.mask_;
- other.mask_ = mask_;
- mask_ = tmp;
- other.entries_.swap(entries_);
+ std::swap(mask_, other.mask_);
+ entries_.swap(other.entries_);
}
int operator[](int key) const {
@@ -128,9 +126,11 @@ class IdMapping {
void DoubleCapacity() {
int new_capacity = capacity() * 2;
+ DCHECK_GE(new_capacity, 0);
std::vector<value_type> entries(new_capacity);
ClearMap(&entries);
mask_ = new_capacity - 1;
+ DCHECK_EQ(mask_ & new_capacity, 0);
entries.swap(entries_);
for (const auto& entry : entries) {
diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc
index cc1ab10..cd6c9dd 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -94,6 +94,7 @@ using kudu::client::KuduInsert;
using kudu::client::KuduSchema;
using kudu::client::KuduSchemaBuilder;
using kudu::client::KuduTable;
+using kudu::client::KuduTableAlterer;
using kudu::client::KuduTableCreator;
using kudu::client::KuduValue;
using kudu::client::sp::shared_ptr;
@@ -2983,7 +2984,7 @@ TEST_F(AdminCliTest, TestRebuildMasterWithTombstones) {
ValueDeleter deleter(&ts_map);
// Find the single tablet replica and tombstone it.
string tablet_id;
- TServerDetails* ts_details;
+ TServerDetails* ts_details = nullptr;
for (const auto& id_and_details : ts_map) {
vector<string> tablet_ids;
ts_details = id_and_details.second;
@@ -3082,7 +3083,7 @@ TEST_P(SecureClusterAdminCliParamTest, TestRebuildMaster) {
FLAGS_num_replicas = 3;
// Create a table and insert some rows
- NO_FATALS(MakeTestTable(kPreRebuildTableName, kNumRows, 3, cluster_.get()));
+ NO_FATALS(MakeTestTable(kPreRebuildTableName, kNumRows, /*num_replicas*/3, cluster_.get()));
// Scan the table to strings so we can check it isn't corrupted post-rebuild.
vector<string> rows;
@@ -3134,7 +3135,61 @@ TEST_P(SecureClusterAdminCliParamTest, TestRebuildMaster) {
ASSERT_OK(client_->DeleteTable(kPreRebuildTableName));
// Make sure we can create a table and write to it.
- NO_FATALS(MakeTestTable(kPostRebuildTableName, kNumRows, 3, cluster_.get()));
+ NO_FATALS(MakeTestTable(kPostRebuildTableName, kNumRows, /*num_replicas*/3, cluster_.get()));
+ NO_FATALS(cv.CheckCluster());
+}
+
+TEST_P(SecureClusterAdminCliParamTest, TestRebuildMasterAndAddColumns) {
+ bool is_secure = GetParam();
+ constexpr const char* kTableName = "rebuild_and_add_columns";
+ constexpr int kNumRows = 10000;
+
+ FLAGS_num_tablet_servers = 3;
+ FLAGS_num_replicas = 3;
+
+ // Create a table and insert some rows
+ NO_FATALS(MakeTestTable(kTableName, kNumRows, /*num_replicas*/3, cluster_.get()));
+
+ // Shut down the master and wipe out its data.
+ NO_FATALS(cluster_->master()->Shutdown());
+ ASSERT_OK(cluster_->master()->DeleteFromDisk());
+
+ // Rebuild the master with the tool.
+ string stdout;
+ ASSERT_OK(RunKuduTool(RebuildMasterCmd(*cluster_, is_secure), &stdout));
+ ASSERT_STR_CONTAINS(stdout,
+ "Rebuilt from 3 tablet servers, of which 0 had errors");
+ ASSERT_STR_CONTAINS(stdout, "Rebuilt from 3 replicas, of which 0 had errors");
+
+ // Restart the master and the tablet servers.
+ // The tablet servers must be restarted so they accept the new master's certs.
+ for (int i = 0; i < cluster_->num_tablet_servers(); i++) {
+ cluster_->tablet_server(i)->Shutdown();
+ }
+ ASSERT_OK(cluster_->Restart());
+
+ // The client has to be rebuilt since there's a new master.
+ KuduClientBuilder builder;
+ ASSERT_OK(cluster_->CreateClient(&builder, &client_));
+
+ // Make sure we can add columns to the table we rebuilt.
+ unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+ for (int i = 0; i < 10; i++) {
+ table_alterer->AddColumn(Substitute("c$0", i))->Type(KuduColumnSchema::INT32);
+ }
+ ASSERT_OK(table_alterer->Alter());
+
+ // Check the altered table is readable.
+ {
+ vector<string> rows;
+ client::sp::shared_ptr<KuduTable> table;
+ ASSERT_OK(client_->OpenTable(kTableName, &table));
+ ScanTableToStrings(table.get(), &rows);
+ }
+
+ // The cluster should still be considered healthy.
+ FLAGS_sasl_protocol_name = kPrincipal;
+ ClusterVerifier cv(cluster_.get());
NO_FATALS(cv.CheckCluster());
}
diff --git a/src/kudu/tools/master_rebuilder.cc b/src/kudu/tools/master_rebuilder.cc
index a5fffa7..27c9e3c 100644
--- a/src/kudu/tools/master_rebuilder.cc
+++ b/src/kudu/tools/master_rebuilder.cc
@@ -19,7 +19,6 @@
#include <cstdint>
#include <functional>
-#include <limits>
#include <map>
#include <memory>
#include <ostream>
@@ -198,9 +197,13 @@ void MasterRebuilder::CreateTable(const ListTabletsResponsePB::StatusAndSchemaPB
table_metadata->mutable_schema()->CopyFrom(replica.schema());
table_metadata->mutable_partition_schema()->CopyFrom(replica.partition_schema());
- // We can't tell what the next column id should be, so we guess something so
- // large we're almost certainly OK.
- table_metadata->set_next_column_id(std::numeric_limits<int32_t>::max() / 2);
+ int32_t max_column_id = 0;
+ for (const auto& column : replica.schema().columns()) {
+ if (column.has_id() && max_column_id < column.id()) {
+ max_column_id = column.id();
+ }
+ }
+ table_metadata->set_next_column_id(max_column_id + 1);
table_metadata->set_state(SysTablesEntryPB::RUNNING);
table_metadata->set_state_msg("reconstructed by MasterRebuilder");
table->mutable_metadata()->CommitMutation();
@@ -213,14 +216,19 @@ Status MasterRebuilder::CheckTableConsistency(
const string& table_name = replica.tablet_status().table_name();
scoped_refptr<TableInfo> table = FindOrDie(tables_by_name_, table_name);
- TableMetadataLock l_table(table.get(), LockMode::READ);
- const SysTablesEntryPB& metadata = table->metadata().state().pb;
+ TableMetadataLock l_table(table.get(), LockMode::WRITE);
+ SysTablesEntryPB* metadata = &table->mutable_metadata()->mutable_dirty()->pb;
- // Check the schemas match.
- Schema schema_from_table;
+ // Update next_column_id if needed.
Schema schema_from_replica;
- RETURN_NOT_OK(SchemaFromPB(metadata.schema(), &schema_from_table));
RETURN_NOT_OK(SchemaFromPB(replica.schema(), &schema_from_replica));
+ if (schema_from_replica.max_col_id() + 1 > metadata->next_column_id()) {
+ metadata->set_next_column_id(schema_from_replica.max_col_id() + 1);
+ }
+
+ // Check the schemas match.
+ Schema schema_from_table;
+ RETURN_NOT_OK(SchemaFromPB(metadata->schema(), &schema_from_table));
if (schema_from_table != schema_from_replica) {
LOG(WARNING) << Substitute("Schema mismatch for tablet $0 of table $1", tablet_id, table_name);
LOG(WARNING) << Substitute("Table schema: $0", schema_from_table.ToString());
@@ -231,7 +239,7 @@ Status MasterRebuilder::CheckTableConsistency(
// Check the partition schemas match.
PartitionSchema pschema_from_table;
PartitionSchema pschema_from_replica;
- RETURN_NOT_OK(PartitionSchema::FromPB(metadata.partition_schema(),
+ RETURN_NOT_OK(PartitionSchema::FromPB(metadata->partition_schema(),
schema_from_table,
&pschema_from_table));
RETURN_NOT_OK(PartitionSchema::FromPB(replica.partition_schema(),
diff --git a/src/kudu/tools/master_rebuilder.h b/src/kudu/tools/master_rebuilder.h
index 6c95483..d8d5110 100644
--- a/src/kudu/tools/master_rebuilder.h
+++ b/src/kudu/tools/master_rebuilder.h
@@ -102,6 +102,8 @@ class MasterRebuilder {
// metadata for its table or tablet, returning an error if the input
// 'replica' is inconsistent with existing metadata collected for the same
// tablet or table.
+ // Before checking inconsistent, table's next_column_id will be updated if
+ // found a larger one in 'replica'.
Status CheckTableConsistency(const tserver::ListTabletsResponsePB::StatusAndSchemaPB& replica);
Status CheckTabletConsistency(const tserver::ListTabletsResponsePB::StatusAndSchemaPB& replica);