Skip to content

[enhance](partitionid) check partition id to avoid unexpected behavior #28045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiaokang merged 9 commits into from
Dec 6, 2023
Merged

[enhance](partitionid) check partition id to avoid unexpected behavior #28045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9302d17
[enhance](partitionid) check partition id to avoid unexpected behavior
dataroaring Dec 6, 2023
4f0cbcc
format
dataroaring Dec 6, 2023
bbc060d
fix
dataroaring Dec 6, 2023
d38fc2e
format
dataroaring Dec 6, 2023
ba37965
fix
dataroaring Dec 6, 2023
1bd15f6
fix
dataroaring Dec 6, 2023
2b958c6
fix ut
dataroaring Dec 6, 2023
7a4eb19
fix ut
dataroaring Dec 6, 2023
da882e4
fix ut
dataroaring Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions be/src/olap/data_dir.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,15 +506,22 @@ Status DataDir::load() {
_meta, rowset_meta->partition_id(), rowset_meta->txn_id(),
rowset_meta->tablet_id(), rowset_meta->tablet_schema_hash(),
rowset_meta->tablet_uid(), rowset_meta->load_id(), rowset, true);
if (!commit_txn_status && !commit_txn_status.is<PUSH_TRANSACTION_ALREADY_EXIST>()) {
LOG(WARNING) << "failed to add committed rowset: " << rowset_meta->rowset_id()
<< " to tablet: " << rowset_meta->tablet_id()
<< " for txn: " << rowset_meta->txn_id();
} else {
if (commit_txn_status || commit_txn_status.is<PUSH_TRANSACTION_ALREADY_EXIST>()) {
LOG(INFO) << "successfully to add committed rowset: " << rowset_meta->rowset_id()
<< " to tablet: " << rowset_meta->tablet_id()
<< " schema hash: " << rowset_meta->tablet_schema_hash()
<< " for txn: " << rowset_meta->txn_id();
} else if (commit_txn_status.is<ErrorCode::INTERNAL_ERROR>()) {
LOG(WARNING) << "failed to add committed rowset: " << rowset_meta->rowset_id()
<< " to tablet: " << rowset_meta->tablet_id()
<< " for txn: " << rowset_meta->txn_id()
<< " error: " << commit_txn_status;
return commit_txn_status;
} else {
LOG(WARNING) << "failed to add committed rowset: " << rowset_meta->rowset_id()
<< " to tablet: " << rowset_meta->tablet_id()
<< " for txn: " << rowset_meta->txn_id()
<< " error: " << commit_txn_status;
}
} else if (rowset_meta->rowset_state() == RowsetStatePB::VISIBLE &&
rowset_meta->tablet_uid() == tablet->tablet_uid()) {
Expand Down
6 changes: 6 additions & 0 deletions be/src/olap/rowset/rowset_meta_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ Status RowsetMetaManager::get_json_rowset_meta(OlapMeta* meta, TabletUid tablet_
}
Status RowsetMetaManager::save(OlapMeta* meta, TabletUid tablet_uid, const RowsetId& rowset_id,
const RowsetMetaPB& rowset_meta_pb, bool enable_binlog) {
if (rowset_meta_pb.partition_id() <= 0) {
LOG(WARNING) << "invalid partition id " << rowset_meta_pb.partition_id() << " tablet "
<< rowset_meta_pb.tablet_id();
return Status::InternalError("invaid partition id {} tablet {}",
rowset_meta_pb.partition_id(), rowset_meta_pb.tablet_id());
}
if (enable_binlog) {
return _save_with_binlog(meta, tablet_uid, rowset_id, rowset_meta_pb);
} else {
Expand Down
5 changes: 5 additions & 0 deletions be/src/olap/tablet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,11 @@ void WriteCooldownMetaExecutors::WriteCooldownMetaExecutors::submit(TabletShared

TabletSharedPtr Tablet::create_tablet_from_meta(TabletMetaSharedPtr tablet_meta,
DataDir* data_dir) {
if (tablet_meta->partition_id() <= 0) {
LOG(WARNING) << "invalid partition id " << tablet_meta->partition_id() << ", tablet "
<< tablet_meta->tablet_id();
return nullptr;
Copy link
Contributor

@xiaokang xiaokang Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it cause NPE for caller?

}
return std::make_shared<Tablet>(tablet_meta, data_dir);
}

Expand Down
6 changes: 6 additions & 0 deletions be/src/olap/tablet_meta_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ Status TabletMetaManager::save(DataDir* store, TTabletId tablet_id, TSchemaHash
std::string key = fmt::format("{}{}_{}", header_prefix, tablet_id, schema_hash);
std::string value;
tablet_meta->serialize(&value);
if (tablet_meta->partition_id() <= 0) {
LOG(WARNING) << "invalid partition id " << tablet_meta->partition_id() << " tablet "
<< tablet_meta->tablet_id();
return Status::InternalError("invaid partition id {} tablet {}",
tablet_meta->partition_id(), tablet_meta->tablet_id());
}
OlapMeta* meta = store->get_meta();
VLOG_NOTICE << "save tablet meta"
<< ", key:" << key << ", meta length:" << value.length();
Expand Down
7 changes: 4 additions & 3 deletions be/src/olap/txn_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,10 @@ Status TxnManager::commit_txn(OlapMeta* meta, TPartitionId partition_id,
const PUniqueId& load_id, const RowsetSharedPtr& rowset_ptr,
bool is_recovery) {
if (partition_id < 1 || transaction_id < 1 || tablet_id < 1) {
LOG(FATAL) << "invalid commit req "
<< " partition_id=" << partition_id << " transaction_id=" << transaction_id
<< " tablet_id=" << tablet_id;
LOG(WARNING) << "invalid commit req "
<< " partition_id=" << partition_id << " transaction_id=" << transaction_id
<< " tablet_id=" << tablet_id;
return Status::InternalError("invalid partition id");
}

pair<int64_t, int64_t> key(partition_id, transaction_id);
Expand Down
2 changes: 2 additions & 0 deletions be/test/olap/delete_handler_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ static void tear_down() {
static void set_default_create_tablet_request(TCreateTabletReq* request) {
request->tablet_id = 10003;
request->__set_version(1);
request->partition_id = 10004;
request->tablet_schema.schema_hash = 270068375;
request->tablet_schema.short_key_column_count = 2;
request->tablet_schema.keys_type = TKeysType::AGG_KEYS;
Expand Down Expand Up @@ -185,6 +186,7 @@ static void set_default_create_tablet_request(TCreateTabletReq* request) {

static void set_create_duplicate_tablet_request(TCreateTabletReq* request) {
request->tablet_id = 10009;
request->partition_id = 10010;
request->__set_version(1);
request->tablet_schema.schema_hash = 270068376;
request->tablet_schema.short_key_column_count = 2;
Expand Down
2 changes: 2 additions & 0 deletions be/test/olap/delta_writer_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ static void tear_down() {
static void create_tablet_request(int64_t tablet_id, int32_t schema_hash,
TCreateTabletReq* request) {
request->tablet_id = tablet_id;
request->partition_id = 1000;
request->__set_version(1);
request->tablet_schema.schema_hash = schema_hash;
request->tablet_schema.short_key_column_count = 6;
Expand Down Expand Up @@ -264,6 +265,7 @@ static void create_tablet_request_with_sequence_col(int64_t tablet_id, int32_t s
TCreateTabletReq* request,
bool enable_mow = false) {
request->tablet_id = tablet_id;
request->partition_id = 10000;
request->__set_version(1);
request->tablet_schema.schema_hash = schema_hash;
request->tablet_schema.short_key_column_count = 2;
Expand Down
1 change: 1 addition & 0 deletions be/test/olap/engine_storage_migration_task_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ static void tear_down() {
static void create_tablet_request_with_sequence_col(int64_t tablet_id, int32_t schema_hash,
TCreateTabletReq* request) {
request->tablet_id = tablet_id;
request->partition_id = 20001;
request->__set_version(1);
request->tablet_schema.schema_hash = schema_hash;
request->tablet_schema.short_key_column_count = 2;
Expand Down
1 change: 1 addition & 0 deletions be/test/olap/remote_rowset_gc_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class RemoteRowsetGcTest : public testing::Test {
static void create_tablet_request_with_sequence_col(int64_t tablet_id, int32_t schema_hash,
TCreateTabletReq* request) {
request->tablet_id = tablet_id;
request->partition_id = 1000;
request->__set_version(1);
request->tablet_schema.schema_hash = schema_hash;
request->tablet_schema.short_key_column_count = 2;
Expand Down
1 change: 1 addition & 0 deletions be/test/olap/tablet_cooldown_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ class TabletCooldownTest : public testing::Test {
static void create_tablet_request_with_sequence_col(int64_t tablet_id, int32_t schema_hash,
TCreateTabletReq* request) {
request->tablet_id = tablet_id;
request->partition_id = 1000;
request->__set_version(1);
request->tablet_schema.schema_hash = schema_hash;
request->tablet_schema.short_key_column_count = 2;
Expand Down
3 changes: 3 additions & 0 deletions be/test/olap/tablet_mgr_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ TEST_F(TabletMgrTest, CreateTablet) {
create_tablet_req.__set_tablet_schema(tablet_schema);
create_tablet_req.__set_tablet_id(111);
create_tablet_req.__set_version(2);
create_tablet_req.__set_partition_id(1000);
std::vector<DataDir*> data_dirs;
data_dirs.push_back(_data_dir);
RuntimeProfile profile("CreateTablet");
Expand Down Expand Up @@ -167,6 +168,7 @@ TEST_F(TabletMgrTest, CreateTabletWithSequence) {
TCreateTabletReq create_tablet_req;
create_tablet_req.__set_tablet_schema(tablet_schema);
create_tablet_req.__set_tablet_id(111);
create_tablet_req.__set_partition_id(1000);
create_tablet_req.__set_version(2);
std::vector<DataDir*> data_dirs;
data_dirs.push_back(_data_dir);
Expand Down Expand Up @@ -210,6 +212,7 @@ TEST_F(TabletMgrTest, DropTablet) {
TCreateTabletReq create_tablet_req;
create_tablet_req.__set_tablet_schema(tablet_schema);
create_tablet_req.__set_tablet_id(111);
create_tablet_req.__set_partition_id(1000);
create_tablet_req.__set_version(2);
std::vector<DataDir*> data_dirs;
data_dirs.push_back(_data_dir);
Expand Down