You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2022/02/16 21:27:59 UTC
[kudu] branch master updated (31a9b01 -> 5557c48)
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from 31a9b01 [thread] Small refactor to improve ThreadMgr performance
new 9f164b3 [tool] fix a command bug, cmd: kudu wal dump ...
new 5557c48 [schema] use operator==() instead of Equals()
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/kudu/client/client.cc | 8 +-
src/kudu/client/schema.cc | 3 +-
src/kudu/common/generic_iterators.cc | 4 +-
src/kudu/common/row.h | 6 +-
src/kudu/common/schema-test.cc | 88 ++++++----
src/kudu/common/schema.h | 51 +++---
src/kudu/common/wire_protocol-test-util.h | 41 ++++-
src/kudu/integration-tests/alter_table-test.cc | 4 +-
src/kudu/master/master-test.cc | 2 +-
src/kudu/master/sys_catalog.cc | 2 +-
src/kudu/tablet/compaction-test.cc | 2 +-
src/kudu/tablet/tablet.cc | 2 +-
src/kudu/tablet/tablet_metadata.cc | 2 +-
src/kudu/tools/kudu-tool-test.cc | 218 +++++++++++++++++++++++++
src/kudu/tools/tool_action_common.cc | 14 +-
src/kudu/tserver/tablet_service.cc | 4 +-
16 files changed, 367 insertions(+), 84 deletions(-)
[kudu] 02/02: [schema] use operator==() instead of Equals()
Posted by al...@apache.org.
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
commit 5557c488e1c4264c20b764235b4830456f73ce3b
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Feb 15 13:43:53 2022 -0800
[schema] use operator==() instead of Equals()
While looking at the code in src/kudu/common/schema.h in the scope of
reviewing a prior changelist, I noticed that the comparison options
for Schema::Equals() were used only in the test scenarios. I took
the liberty and moved the Equals() method into the test code and
replaced it with the operator== at call sites. This patch includes
other minor cleanup, but overall the patch doesn't contain any
functional changes.
Change-Id: Ib0e3d75d216241efec49b64233198eafe52871cb
Reviewed-on: http://gerrit.cloudera.org:8080/18237
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/client/client.cc | 8 +--
src/kudu/client/schema.cc | 3 +-
src/kudu/common/generic_iterators.cc | 4 +-
src/kudu/common/row.h | 6 +-
src/kudu/common/schema-test.cc | 88 +++++++++++++++++---------
src/kudu/common/schema.h | 51 ++++++---------
src/kudu/integration-tests/alter_table-test.cc | 4 +-
src/kudu/master/master-test.cc | 2 +-
src/kudu/master/sys_catalog.cc | 2 +-
src/kudu/tablet/compaction-test.cc | 2 +-
src/kudu/tablet/tablet.cc | 2 +-
src/kudu/tablet/tablet_metadata.cc | 2 +-
src/kudu/tserver/tablet_service.cc | 4 +-
13 files changed, 98 insertions(+), 80 deletions(-)
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index e26a46c..4b22424 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -1515,13 +1515,13 @@ KuduTableAlterer* KuduTableAlterer::AddRangePartitionWithDimension(
data_->status_ = Status::InvalidArgument("range partition bounds may not be null");
return this;
}
- if (!lower_bound->schema()->Equals(*upper_bound->schema())) {
+ if (*lower_bound->schema() != *upper_bound->schema()) {
data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas");
return this;
}
if (data_->schema_ == nullptr) {
data_->schema_ = lower_bound->schema();
- } else if (!lower_bound->schema()->Equals(*data_->schema_)) {
+ } else if (*lower_bound->schema() != *data_->schema_) {
data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas");
return this;
}
@@ -1547,13 +1547,13 @@ KuduTableAlterer* KuduTableAlterer::DropRangePartition(
data_->status_ = Status::InvalidArgument("range partition bounds may not be null");
return this;
}
- if (!lower_bound->schema()->Equals(*upper_bound->schema())) {
+ if (*lower_bound->schema() != *upper_bound->schema()) {
data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas");
return this;
}
if (data_->schema_ == nullptr) {
data_->schema_ = lower_bound->schema();
- } else if (!lower_bound->schema()->Equals(*data_->schema_)) {
+ } else if (*lower_bound->schema() != *data_->schema_) {
data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas");
return this;
}
diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc
index 8c97e55..b53b5fe 100644
--- a/src/kudu/client/schema.cc
+++ b/src/kudu/client/schema.cc
@@ -878,8 +878,7 @@ Status KuduSchema::Reset(const vector<KuduColumnSchema>& columns, int key_column
}
bool KuduSchema::operator==(const KuduSchema& rhs) const {
- return this == &rhs ||
- (schema_ && rhs.schema_ && schema_->Equals(*rhs.schema_));
+ return this == &rhs || (schema_ && rhs.schema_ && (*schema_ == *rhs.schema_));
}
bool KuduSchema::operator!=(const KuduSchema& rhs) const {
diff --git a/src/kudu/common/generic_iterators.cc b/src/kudu/common/generic_iterators.cc
index c2c3851..ad9fa17 100644
--- a/src/kudu/common/generic_iterators.cc
+++ b/src/kudu/common/generic_iterators.cc
@@ -616,7 +616,7 @@ Status MergeIterator::Init(ScanSpec *spec) {
finished_iter_stats_by_col_.resize(schema_->num_columns());
#ifndef NDEBUG
for (const auto& s : states_) {
- if (!s.schema().Equals(*schema_)) {
+ if (s.schema() != *schema_) {
return Status::InvalidArgument(
Substitute("Schemas do not match: $0 vs. $1",
schema_->ToString(), s.schema().ToString()));
@@ -1008,7 +1008,7 @@ Status UnionIterator::Init(ScanSpec *spec) {
finished_iter_stats_by_col_.resize(schema_->num_columns());
#ifndef NDEBUG
for (const auto& i : iters_) {
- if (!i.iter->schema().Equals(*schema_)) {
+ if (i.iter->schema() != *schema_) {
return Status::InvalidArgument(
Substitute("Schemas do not match: $0 vs. $1",
schema_->ToString(), i.iter->schema().ToString()));
diff --git a/src/kudu/common/row.h b/src/kudu/common/row.h
index a2e3756..aa63241 100644
--- a/src/kudu/common/row.h
+++ b/src/kudu/common/row.h
@@ -144,7 +144,7 @@ class RowProjector {
// The two Schema pointers must remain valid for the lifetime of this object.
RowProjector(const Schema* base_schema, const Schema* projection)
: base_schema_(base_schema), projection_(projection),
- is_identity_(base_schema->Equals(*projection)) {
+ is_identity_(*base_schema == *projection) {
}
// Initialize the projection mapping with the specified base_schema and projection
@@ -157,7 +157,7 @@ class RowProjector {
projection_ = projection;
base_cols_mapping_.clear();
projection_defaults_.clear();
- is_identity_ = base_schema->Equals(*projection);
+ is_identity_ = (*base_schema == *projection);
return Init();
}
@@ -275,7 +275,7 @@ class DeltaProjector {
// of the object.
DeltaProjector(const Schema* delta_schema, const Schema* projection)
: delta_schema_(delta_schema), projection_(projection),
- is_identity_(delta_schema->Equals(*projection)) {
+ is_identity_(*delta_schema == *projection) {
}
Status Init() {
diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc
index 251402b..ae79cf8 100644
--- a/src/kudu/common/schema-test.cc
+++ b/src/kudu/common/schema-test.cc
@@ -21,7 +21,6 @@
#include <cstdint>
#include <string>
#include <tuple> // IWYU pragma: keep
-#include <unordered_map>
#include <utility>
#include <vector>
@@ -44,19 +43,52 @@
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
-namespace kudu {
-namespace tablet {
-
using std::string;
-using std::unordered_map;
using std::vector;
using strings::Substitute;
+namespace kudu {
+
+// Return true if the schemas have exactly the same set of columns
+// and respective types.
+bool EqualSchemas(const Schema& lhs, const Schema& rhs) {
+ if (lhs != rhs) {
+ return false;
+ }
+
+ if (lhs.num_key_columns_ != rhs.num_key_columns_) {
+ return false;
+ }
+ if (lhs.num_columns() != rhs.num_columns()) {
+ return false;
+ }
+ for (size_t i = 0; i < rhs.num_columns(); ++i) {
+ if (!lhs.cols_[i].Equals(rhs.cols_[i])) {
+ return false;
+ }
+ }
+
+ if (lhs.has_column_ids() != rhs.has_column_ids()) {
+ return false;
+ }
+ if (lhs.has_column_ids()) {
+ if (lhs.col_ids_ != rhs.col_ids_) {
+ return false;
+ }
+ if (lhs.max_col_id() != rhs.max_col_id()) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+namespace tablet {
+
// Copy a row and its referenced data into the given Arena.
-static Status CopyRowToArena(const Slice &row,
- const Schema &schema,
- Arena *dst_arena,
- ContiguousRow *copied) {
+static Status CopyRowToArena(const Slice& row,
+ Arena* dst_arena,
+ ContiguousRow* copied) {
Slice row_data;
// Copy the direct row data to arena
@@ -65,8 +97,7 @@ static Status CopyRowToArena(const Slice &row,
}
copied->Reset(row_data.mutable_data());
- RETURN_NOT_OK(RelocateIndirectDataToArena(copied, dst_arena));
- return Status::OK();
+ return RelocateIndirectDataToArena(copied, dst_arena);
}
class TestSchema : public KuduTest {};
@@ -159,11 +190,10 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) {
vector<ColumnSchema> cols = { col1, col2, col3 };
vector<ColumnId> ids = { ColumnId(0), ColumnId(1), ColumnId(2) };
- const int kNumKeyCols = 1;
+ constexpr int kNumKeyCols = 1;
- Schema schema = GetParam() == INCLUDE_COL_IDS ?
- Schema(cols, ids, kNumKeyCols) :
- Schema(cols, kNumKeyCols);
+ const auto& schema = GetParam() == INCLUDE_COL_IDS
+ ? Schema(cols, ids, kNumKeyCols) : Schema(cols, kNumKeyCols);
NO_FATALS(check_schema(schema));
@@ -172,7 +202,7 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) {
{
Schema copied_schema = schema;
NO_FATALS(check_schema(copied_schema));
- ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL));
+ ASSERT_TRUE(EqualSchemas(schema, copied_schema));
// Move-assign to 'moved_to_schema' from 'copied_schema' and then let
// 'copied_schema' go out of scope to make sure none of the 'moved_schema'
@@ -184,18 +214,18 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) {
copied_schema.ToString(); // NOLINT(*)
}
NO_FATALS(check_schema(moved_schema));
- ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL));
+ ASSERT_TRUE(EqualSchemas(schema, moved_schema));
// Check copy- and move-construction.
{
Schema copied_schema(schema);
NO_FATALS(check_schema(copied_schema));
- ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL));
+ ASSERT_TRUE(EqualSchemas(schema, copied_schema));
Schema moved_schema(std::move(copied_schema));
copied_schema.ToString(); // NOLINT(*)
NO_FATALS(check_schema(moved_schema));
- ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL));
+ ASSERT_TRUE(EqualSchemas(schema, moved_schema));
}
}
@@ -254,10 +284,10 @@ TEST_F(TestSchema, TestSchemaEqualsWithDecimal) {
Schema schema_17_10({ col1, col_17_10 }, 1);
Schema schema_17_9({ col1, col_17_9 }, 1);
- EXPECT_TRUE(schema_18_10.Equals(schema_18_10));
- EXPECT_FALSE(schema_18_10.Equals(schema_18_9));
- EXPECT_FALSE(schema_18_10.Equals(schema_17_10));
- EXPECT_FALSE(schema_18_10.Equals(schema_17_9));
+ EXPECT_EQ(schema_18_10, schema_18_10);
+ EXPECT_NE(schema_18_10, schema_18_9);
+ EXPECT_NE(schema_18_10, schema_17_10);
+ EXPECT_NE(schema_18_10, schema_17_9);
}
TEST_F(TestSchema, TestColumnSchemaEquals) {
@@ -294,14 +324,14 @@ TEST_F(TestSchema, TestSchemaEquals) {
ColumnSchema("col2", UINT32),
ColumnSchema("col3", UINT32, false) },
2);
- ASSERT_FALSE(schema1.Equals(schema2));
+ ASSERT_NE(schema1, schema2);
ASSERT_TRUE(schema1.KeyEquals(schema1));
ASSERT_TRUE(schema1.KeyEquals(schema2, ColumnSchema::COMPARE_TYPE));
ASSERT_FALSE(schema1.KeyEquals(schema2, ColumnSchema::COMPARE_NAME));
ASSERT_TRUE(schema1.KeyTypeEquals(schema2));
ASSERT_FALSE(schema2.KeyTypeEquals(schema3));
- ASSERT_FALSE(schema3.Equals(schema4));
- ASSERT_TRUE(schema4.Equals(schema4));
+ ASSERT_NE(schema3, schema4);
+ ASSERT_EQ(schema4, schema4);
ASSERT_TRUE(schema3.KeyEquals(schema4, ColumnSchema::COMPARE_NAME_AND_TYPE));
}
@@ -470,7 +500,7 @@ TEST_F(TestSchema, TestGetMappedReadProjection) {
ASSERT_EQ(1, mapped.num_key_columns());
ASSERT_EQ(2, mapped.num_columns());
ASSERT_TRUE(mapped.has_column_ids());
- ASSERT_FALSE(mapped.Equals(projection, Schema::COMPARE_ALL));
+ ASSERT_FALSE(EqualSchemas(mapped, projection));
// The column id for the 'key' column in the mapped projection should match
// the one from the tablet schema.
@@ -521,7 +551,7 @@ TEST_F(TestSchema, TestRowOperations) {
rb.AddUint32(3);
rb.AddInt32(-3);
ContiguousRow row_a(&schema);
- ASSERT_OK(CopyRowToArena(rb.data(), schema, &arena, &row_a));
+ ASSERT_OK(CopyRowToArena(rb.data(), &arena, &row_a));
rb.Reset();
rb.AddString(string("row_b_1"));
@@ -529,7 +559,7 @@ TEST_F(TestSchema, TestRowOperations) {
rb.AddUint32(3);
rb.AddInt32(-3);
ContiguousRow row_b(&schema);
- ASSERT_OK(CopyRowToArena(rb.data(), schema, &arena, &row_b));
+ ASSERT_OK(CopyRowToArena(rb.data(), &arena, &row_b));
ASSERT_GT(schema.Compare(row_b, row_a), 0);
ASSERT_LT(schema.Compare(row_a, row_b), 0);
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index c00e66d..725a0e8 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -50,12 +50,12 @@ namespace kudu {
class Schema;
} // namespace kudu
-// Check that two schemas are equal, yielding a useful error message in the case that
-// they are not.
+// Check that two schemas are equal, yielding a useful error message
+// if they are not.
#define DCHECK_SCHEMA_EQ(s1, s2) \
do { \
- DCHECK((s1).Equals((s2))) << "Schema " << (s1).ToString() \
- << " does not match " << (s2).ToString(); \
+ DCHECK((s1) == (s2)) << "Schema " << (s1).ToString() \
+ << " does not match " << (s2).ToString(); \
} while (0)
#define DCHECK_KEY_PROJECTION_SCHEMA_EQ(s1, s2) \
@@ -787,42 +787,28 @@ class Schema {
// so should only be used when necessary for output.
std::string ToString(ToStringMode mode = ToStringMode::WITH_COLUMN_IDS) const;
- // Compare column ids in Equals() method.
- enum SchemaComparisonType {
- COMPARE_COLUMNS = 1 << 0,
- COMPARE_COLUMN_IDS = 1 << 1,
-
- COMPARE_ALL = COMPARE_COLUMNS | COMPARE_COLUMN_IDS
- };
-
- // Return true if the schemas have exactly the same set of columns
- // and respective types.
- bool Equals(const Schema& other, SchemaComparisonType flags = COMPARE_COLUMNS) const {
- if (this == &other) return true;
+ bool operator==(const Schema& other) const {
+ if (this == &other) {
+ return true;
+ }
- if (flags & COMPARE_COLUMNS) {
- if (this->num_key_columns_ != other.num_key_columns_) return false;
- if (this->num_columns() != other.num_columns()) return false;
- for (size_t i = 0; i < other.num_columns(); i++) {
- if (!this->cols_[i].Equals(other.cols_[i])) return false;
- }
+ if (this->num_key_columns_ != other.num_key_columns_) {
+ return false;
}
- if (flags & COMPARE_COLUMN_IDS) {
- if (this->has_column_ids() != other.has_column_ids()) return false;
- if (this->has_column_ids()) {
- if (this->col_ids_ != other.col_ids_) return false;
- if (this->max_col_id() != other.max_col_id()) return false;
+ const size_t num_columns = this->num_columns();
+ if (num_columns != other.num_columns()) {
+ return false;
+ }
+ for (size_t i = 0; i < num_columns; ++i) {
+ if (!this->cols_[i].Equals(other.cols_[i])) {
+ return false;
}
}
return true;
}
- bool operator==(const Schema& other) const {
- return this->Equals(other);
- }
-
bool operator!=(const Schema& other) const {
return !(*this == other);
}
@@ -967,6 +953,9 @@ class Schema {
friend class SchemaBuilder;
+ // 'Deep' compare two schemas: this is used by Schema's unit tests.
+ friend bool EqualSchemas(const Schema&, const Schema&);
+
std::vector<ColumnSchema> cols_;
size_t num_key_columns_;
std::vector<ColumnId> col_ids_;
diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc
index 0c2e79f..c356bda 100644
--- a/src/kudu/integration-tests/alter_table-test.cc
+++ b/src/kudu/integration-tests/alter_table-test.cc
@@ -307,8 +307,8 @@ class AlterTableTest : public KuduTest {
} else {
ASSERT_EQ(first_node_replica->tablet()->tablet_id(),
cur_node_replica->tablet()->tablet_id());
- ASSERT_TRUE(first_node_replica->tablet()->schema()->Equals(
- *(cur_node_replica->tablet()->schema())));
+ ASSERT_EQ(*(first_node_replica->tablet()->schema()),
+ *(cur_node_replica->tablet()->schema()));
if (verify_row_count == VerifyRowCount::kEnable) {
uint64_t cur_count = 0;
ASSERT_OK(cur_node_replica->CountLiveRows(&cur_count));
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index 289c12f..537686b 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -1398,7 +1398,7 @@ TEST_F(MasterTest, TestGetTableSchemaIsAtomicWithCreateTable) {
} else {
Schema receivedSchema;
CHECK_OK(SchemaFromPB(resp.schema(), &receivedSchema));
- CHECK(kTableSchema.Equals(receivedSchema)) <<
+ CHECK(kTableSchema == receivedSchema) <<
strings::Substitute("$0 not equal to $1",
kTableSchema.ToString(), receivedSchema.ToString());
CHECK_EQ(kTableName, resp.table_name());
diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc
index 74662ce..0d417a5 100644
--- a/src/kudu/master/sys_catalog.cc
+++ b/src/kudu/master/sys_catalog.cc
@@ -252,7 +252,7 @@ Status SysCatalogTable::Load(FsManager *fs_manager) {
RETURN_NOT_OK(tablet::TabletMetadata::Load(fs_manager, kSysCatalogTabletId, &metadata));
// Verify that the schema is the current one
- if (!metadata->schema().Equals(BuildTableSchema())) {
+ if (metadata->schema() != BuildTableSchema()) {
// TODO: In this case we probably should execute the migration step.
return(Status::Corruption("Unexpected schema", metadata->schema().ToString()));
}
diff --git a/src/kudu/tablet/compaction-test.cc b/src/kudu/tablet/compaction-test.cc
index 4ccba4c..654b95c 100644
--- a/src/kudu/tablet/compaction-test.cc
+++ b/src/kudu/tablet/compaction-test.cc
@@ -184,7 +184,7 @@ class TestCompaction : public KuduRowSetTest {
int row_key,
int32_t val) {
BuildRow(row_key, val);
- if (!mrs->schema().Equals(*row_builder_.schema())) {
+ if (*row_builder_.schema() != mrs->schema()) {
// The MemRowSet is not projecting the row, so must be done by the caller
RowProjector projector(row_builder_.schema(), &mrs->schema());
uint8_t rowbuf[ContiguousRowHelper::row_size(mrs->schema())];
diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index cbeb2ff..28bb0cf 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -1565,7 +1565,7 @@ Status Tablet::AlterSchema(AlterSchemaOpState* op_state) {
std::lock_guard<Semaphore> lock(rowsets_flush_sem_);
// If the current version >= new version, there is nothing to do.
- bool same_schema = schema()->Equals(*op_state->schema());
+ const bool same_schema = (*schema() == *op_state->schema());
if (metadata_->schema_version() >= op_state->schema_version()) {
const string msg =
Substitute("Skipping requested alter to schema version $0, tablet already "
diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc
index aabcb7d..0a36f1c 100644
--- a/src/kudu/tablet/tablet_metadata.cc
+++ b/src/kudu/tablet/tablet_metadata.cc
@@ -167,7 +167,7 @@ Status TabletMetadata::LoadOrCreate(FsManager* fs_manager,
scoped_refptr<TabletMetadata>* metadata) {
Status s = Load(fs_manager, tablet_id, metadata);
if (s.ok()) {
- if (!(*metadata)->schema().Equals(schema)) {
+ if ((*metadata)->schema() != schema) {
return Status::Corruption(Substitute("Schema on disk ($0) does not "
"match expected schema ($1)", (*metadata)->schema().ToString(),
schema.ToString()));
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index d37bf08..16c73ba 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -1170,8 +1170,8 @@ void TabletServiceAdminImpl::AlterSchema(const AlterSchemaRequestPB* req,
return;
}
- Schema tablet_schema = replica->tablet_metadata()->schema();
- if (req_schema.Equals(tablet_schema)) {
+ const auto& tablet_schema = replica->tablet_metadata()->schema();
+ if (req_schema == tablet_schema) {
context->RespondSuccess();
return;
}
[kudu] 01/02: [tool] fix a command bug, cmd: kudu wal dump ...
Posted by al...@apache.org.
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
commit 9f164b3c542a2e44c963c43488b4fc5b0d0f0f65
Author: shenxingwuying <sh...@gmail.com>
AuthorDate: Thu Jan 20 23:04:19 2022 +0800
[tool] fix a command bug, cmd: kudu wal dump ...
kudu wal dump command will interrupt after reading alter schema entry.
Change-Id: I27acc71597d038cafbbe687117bddb1ce16576c0
Reviewed-on: http://gerrit.cloudera.org:8080/18169
Tested-by: Kudu Jenkins
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/common/wire_protocol-test-util.h | 41 +++++-
src/kudu/tools/kudu-tool-test.cc | 218 ++++++++++++++++++++++++++++++
src/kudu/tools/tool_action_common.cc | 14 +-
3 files changed, 269 insertions(+), 4 deletions(-)
diff --git a/src/kudu/common/wire_protocol-test-util.h b/src/kudu/common/wire_protocol-test-util.h
index a5a0412..2fcf03b 100644
--- a/src/kudu/common/wire_protocol-test-util.h
+++ b/src/kudu/common/wire_protocol-test-util.h
@@ -20,6 +20,7 @@
#include "kudu/common/wire_protocol.h"
+#include <map>
#include <string>
#include "kudu/common/partial_row.h"
@@ -35,6 +36,13 @@ inline Schema GetSimpleTestSchema() {
1);
}
+inline void RowAppendColumn(KuduPartialRow* row,
+ const std::map<std::string, std::string>& columns) {
+ for (const auto& column : columns) {
+ CHECK_OK(row->SetStringCopy(column.first.c_str(), column.second.c_str()));
+ }
+}
+
inline void AddTestRowWithNullableStringToPB(RowOperationsPB::Type op_type,
const Schema& schema,
int32_t key,
@@ -46,12 +54,32 @@ inline void AddTestRowWithNullableStringToPB(RowOperationsPB::Type op_type,
CHECK_OK(row.SetInt32("key", key));
CHECK_OK(row.SetInt32("int_val", int_val));
if (string_val) {
- CHECK_OK(row.SetStringCopy("string_val", string_val));
+ RowAppendColumn(&row, {{"string_val", std::string(string_val)}});
}
RowOperationsPBEncoder enc(ops);
enc.Add(op_type, row);
}
+inline void AddTestRowWithNullableColumnsStringToPB(
+ RowOperationsPB::Type op_type, const Schema& schema,
+ int32_t key, int32_t int_val, const char* string_val,
+ const std::map<std::string, std::string>& columns,
+ RowOperationsPB* ops) {
+ DCHECK(schema.initialized());
+ KuduPartialRow row(&schema);
+ CHECK_OK(row.SetInt32("key", key));
+ CHECK_OK(row.SetInt32("int_val", int_val));
+ if (string_val) {
+ RowAppendColumn(&row, {{"string_val", std::string(string_val)}});
+ }
+ if (!columns.empty()) {
+ RowAppendColumn(&row, columns);
+ }
+ RowOperationsPBEncoder enc(ops);
+ enc.Add(op_type, row);
+}
+
+
inline void AddTestRowToPB(RowOperationsPB::Type op_type,
const Schema& schema,
int32_t key,
@@ -61,6 +89,17 @@ inline void AddTestRowToPB(RowOperationsPB::Type op_type,
AddTestRowWithNullableStringToPB(op_type, schema, key, int_val, string_val.c_str(), ops);
}
+inline void AddTestRowToPBAppendColumns(RowOperationsPB::Type op_type,
+ const Schema& schema, int32_t key,
+ int32_t int_val,
+ const std::string& string_val,
+ const std::map<std::string, std::string>& columns,
+ RowOperationsPB* ops) {
+ AddTestRowWithNullableColumnsStringToPB(op_type, schema,
+ key, int_val, string_val.c_str(),
+ columns, ops);
+}
+
inline void AddTestKeyToPB(RowOperationsPB::Type op_type,
const Schema& schema,
int32_t key,
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index 72898c2..2383b2f 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -2128,6 +2128,224 @@ TEST_F(ToolTest, TestWalDump) {
}
}
+TEST_F(ToolTest, TestWalDumpWithAlterSchema) {
+ const string kTestDir = GetTestPath("test");
+ const string kTestTablet = "ffffffffffffffffffffffffffffffff";
+ Schema schema(GetSimpleTestSchema());
+ Schema schema_with_ids(SchemaBuilder(schema).Build());
+
+ FsManager fs(env_, FsManagerOpts(kTestDir));
+ ASSERT_OK(fs.CreateInitialFileSystemLayout());
+ ASSERT_OK(fs.Open());
+
+ const std::string kFirstMessage("this is a test insert");
+ const std::string kAddColumnName1("addcolumn1_addcolumn1");
+ const std::string kAddColumnName2("addcolumn2_addcolumn2");
+ const std::string kAddColumnName1Message("insert a record, after alter schema");
+ const std::string kAddColumnName2Message("upsert a record, after alter schema");
+ {
+ scoped_refptr<Log> log;
+ ASSERT_OK(Log::Open(LogOptions(),
+ &fs,
+ /*file_cache*/nullptr,
+ kTestTablet,
+ schema_with_ids,
+ 0, // schema_version
+ /*metric_entity*/nullptr,
+ &log));
+
+ std::vector<ReplicateRefPtr> replicates;
+ {
+ OpId opid = consensus::MakeOpId(1, 1);
+ ReplicateRefPtr replicate =
+ consensus::make_scoped_refptr_replicate(new ReplicateMsg());
+ replicate->get()->set_op_type(consensus::WRITE_OP);
+ replicate->get()->mutable_id()->CopyFrom(opid);
+ replicate->get()->set_timestamp(1);
+ WriteRequestPB* write = replicate->get()->mutable_write_request();
+ ASSERT_OK(SchemaToPB(schema, write->mutable_schema()));
+ AddTestRowToPB(RowOperationsPB::INSERT, schema,
+ opid.index(), 0, kFirstMessage,
+ write->mutable_row_operations());
+ write->set_tablet_id(kTestTablet);
+ replicates.emplace_back(replicate);
+ }
+ {
+ OpId opid = consensus::MakeOpId(1, 2);
+ ReplicateRefPtr replicate =
+ consensus::make_scoped_refptr_replicate(new ReplicateMsg());
+ replicate->get()->set_op_type(consensus::ALTER_SCHEMA_OP);
+ replicate->get()->mutable_id()->CopyFrom(opid);
+ replicate->get()->set_timestamp(2);
+ tserver::AlterSchemaRequestPB* alter_schema =
+ replicate->get()->mutable_alter_schema_request();
+ SchemaBuilder schema_builder = SchemaBuilder(schema);
+ ASSERT_OK(schema_builder.AddColumn(kAddColumnName1, STRING, true, nullptr, nullptr));
+ ASSERT_OK(schema_builder.AddColumn(kAddColumnName2, STRING, true, nullptr, nullptr));
+ schema = schema_builder.BuildWithoutIds();
+ schema_with_ids = SchemaBuilder(schema).Build();
+ ASSERT_OK(SchemaToPB(schema_with_ids, alter_schema->mutable_schema()));
+ alter_schema->set_tablet_id(kTestTablet);
+ alter_schema->set_schema_version(1);
+ replicates.emplace_back(replicate);
+ }
+ {
+ OpId opid = consensus::MakeOpId(1, 3);
+ ReplicateRefPtr replicate =
+ consensus::make_scoped_refptr_replicate(new ReplicateMsg());
+ replicate->get()->set_op_type(consensus::WRITE_OP);
+ replicate->get()->mutable_id()->CopyFrom(opid);
+ replicate->get()->set_timestamp(3);
+ WriteRequestPB* write = replicate->get()->mutable_write_request();
+ ASSERT_OK(SchemaToPB(schema, write->mutable_schema()));
+ AddTestRowToPBAppendColumns(RowOperationsPB::INSERT, schema,
+ opid.index(), 2, kFirstMessage,
+ {{kAddColumnName1, kAddColumnName1Message}},
+ write->mutable_row_operations());
+ write->set_tablet_id(kTestTablet);
+ replicates.emplace_back(replicate);
+ }
+ {
+ OpId opid = consensus::MakeOpId(1, 4);
+ ReplicateRefPtr replicate =
+ consensus::make_scoped_refptr_replicate(new ReplicateMsg());
+ replicate->get()->set_op_type(consensus::WRITE_OP);
+ replicate->get()->mutable_id()->CopyFrom(opid);
+ replicate->get()->set_timestamp(4);
+ WriteRequestPB* write = replicate->get()->mutable_write_request();
+ ASSERT_OK(SchemaToPB(schema, write->mutable_schema()));
+ AddTestRowToPBAppendColumns(RowOperationsPB::UPSERT, schema,
+ 1, 1111, kFirstMessage,
+ {
+ {kAddColumnName1, kAddColumnName1Message},
+ {kAddColumnName2, kAddColumnName2Message},
+ },
+ write->mutable_row_operations());
+ write->set_tablet_id(kTestTablet);
+ replicates.emplace_back(replicate);
+ }
+
+ Synchronizer s;
+ ASSERT_OK(log->AsyncAppendReplicates(replicates, s.AsStatusCallback()));
+ ASSERT_OK(s.Wait());
+ }
+
+ string wal_path = fs.GetWalSegmentFileName(kTestTablet, 1);
+ string stdout;
+ for (const auto& args : { Substitute("wal dump $0", wal_path),
+ Substitute("local_replica dump wals --fs_wal_dir=$0 $1",
+ kTestDir, kTestTablet)
+ }) {
+ SCOPED_TRACE(args);
+ for (const auto& print_entries : { "true", "1", "yes", "decoded" }) {
+ SCOPED_TRACE(print_entries);
+ NO_FATALS(RunActionStdoutString(Substitute("$0 --print_entries=$1",
+ args, print_entries), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_MATCHES(stdout, "Header:");
+ ASSERT_STR_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_MATCHES(stdout, "ALTER_SCHEMA_OP");
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_NOT_MATCHES(stdout, "t<truncated>");
+ ASSERT_STR_NOT_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_MATCHES(stdout, "Footer:");
+ }
+ for (const auto& print_entries : { "false", "0", "no" }) {
+ SCOPED_TRACE(print_entries);
+ NO_FATALS(RunActionStdoutString(Substitute("$0 --print_entries=$1",
+ args, print_entries), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_MATCHES(stdout, "Header:");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_NOT_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_NOT_MATCHES(stdout, "ALTER_SCHEMA_OP");
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_NOT_MATCHES(stdout, "t<truncated>");
+ ASSERT_STR_NOT_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_MATCHES(stdout, "Footer:");
+ }
+ {
+ NO_FATALS(RunActionStdoutString(Substitute("$0 --print_entries=pb",
+ args), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_MATCHES(stdout, "Header:");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_MATCHES(stdout, "ALTER_SCHEMA_OP");
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_NOT_MATCHES(stdout, "t<truncated>");
+ ASSERT_STR_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_MATCHES(stdout, "Footer:");
+ }
+ {
+ NO_FATALS(RunActionStdoutString(Substitute(
+ "$0 --print_entries=pb --truncate_data=1", args), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_MATCHES(stdout, "Header:");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_NOT_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_NOT_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_MATCHES(stdout, "t<truncated>");
+ ASSERT_STR_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_MATCHES(stdout, "Footer:");
+ }
+ {
+ NO_FATALS(RunActionStdoutString(Substitute(
+ "$0 --print_entries=id", args), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_MATCHES(stdout, "Header:");
+ ASSERT_STR_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_NOT_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_NOT_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_NOT_MATCHES(stdout, "t<truncated>");
+ ASSERT_STR_NOT_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_MATCHES(stdout, "Footer:");
+ }
+ {
+ NO_FATALS(RunActionStdoutString(Substitute(
+ "$0 --print_meta=false", args), &stdout));
+ SCOPED_TRACE(stdout);
+ ASSERT_STR_NOT_MATCHES(stdout, "Header:");
+ ASSERT_STR_MATCHES(stdout, "1\\.1@1");
+ ASSERT_STR_MATCHES(stdout, "1\\.2@2");
+ ASSERT_STR_MATCHES(stdout, "1\\.3@3");
+ ASSERT_STR_MATCHES(stdout, "1\\.4@4");
+ ASSERT_STR_MATCHES(stdout, kFirstMessage);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName1Message);
+ ASSERT_STR_MATCHES(stdout, kAddColumnName2Message);
+ ASSERT_STR_NOT_MATCHES(stdout, "row_operations \\{");
+ ASSERT_STR_NOT_MATCHES(stdout, "Footer:");
+ }
+ }
+}
+
TEST_F(ToolTest, TestLocalReplicaDumpDataDirs) {
constexpr const char* const kTestTablet = "ffffffffffffffffffffffffffffffff";
constexpr const char* const kTestTableId = "test-table";
diff --git a/src/kudu/tools/tool_action_common.cc b/src/kudu/tools/tool_action_common.cc
index a89e0b4..b615b71 100644
--- a/src/kudu/tools/tool_action_common.cc
+++ b/src/kudu/tools/tool_action_common.cc
@@ -74,6 +74,7 @@
#include "kudu/tools/tool.pb.h" // IWYU pragma: keep
#include "kudu/tools/tool_action.h"
#include "kudu/tserver/tserver.pb.h"
+#include "kudu/tserver/tserver_admin.pb.h"
#include "kudu/tserver/tserver_admin.proxy.h" // IWYU pragma: keep
#include "kudu/tserver/tserver_service.proxy.h" // IWYU pragma: keep
#include "kudu/util/async_util.h"
@@ -345,7 +346,7 @@ Status PrintDecodedWriteRequestPB(const string& indent,
return Status::OK();
}
-Status PrintDecoded(const LogEntryPB& entry, const Schema& tablet_schema) {
+Status PrintDecoded(const LogEntryPB& entry, Schema* tablet_schema) {
PrintIdOnly(entry);
const string indent = "\t";
@@ -356,9 +357,16 @@ Status PrintDecoded(const LogEntryPB& entry, const Schema& tablet_schema) {
if (replicate.op_type() == consensus::WRITE_OP) {
RETURN_NOT_OK(PrintDecodedWriteRequestPB(
indent,
- tablet_schema,
+ *tablet_schema,
replicate.write_request(),
replicate.has_request_id() ? &replicate.request_id() : nullptr));
+ } else if (replicate.op_type() == consensus::ALTER_SCHEMA_OP) {
+ if (!replicate.has_alter_schema_request()) {
+ LOG(ERROR) << "read an ALTER_SCHEMA_OP log entry, but has no alter_schema_request";
+ return Status::RuntimeError("ALTER_SCHEMA_OP log entry has no alter_schema_request");
+ }
+ RETURN_NOT_OK(SchemaFromPB(replicate.alter_schema_request().schema(), tablet_schema));
+ cout << indent << SecureShortDebugString(replicate) << endl;
} else {
cout << indent << SecureShortDebugString(replicate) << endl;
}
@@ -555,7 +563,7 @@ Status PrintSegment(const scoped_refptr<ReadableLogSegment>& segment) {
cout << "Entry:\n" << SecureDebugString(*entry);
} else if (print_type == PRINT_DECODED) {
- RETURN_NOT_OK(PrintDecoded(*entry, tablet_schema));
+ RETURN_NOT_OK(PrintDecoded(*entry, &tablet_schema));
} else if (print_type == PRINT_ID) {
PrintIdOnly(*entry);
}