github-actions[bot] commented on code in PR #31055:
URL: https://github.com/apache/doris/pull/31055#discussion_r1495359187
##########
be/src/olap/schema_change.cpp:
##########
@@ -934,80 +944,81 @@
}
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.insert(new_tablet->tablet_id());
+ _tablet_ids_in_converting.insert(_new_tablet->tablet_id());
}
int64_t real_alter_version = 0;
+ sc_params.enable_unique_key_merge_on_write =
+ _new_tablet->enable_unique_key_merge_on_write();
res = _convert_historical_rowsets(sc_params, &real_alter_version);
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.erase(new_tablet->tablet_id());
+ _tablet_ids_in_converting.erase(_new_tablet->tablet_id());
}
if (!res) {
break;
}
- if (new_tablet->keys_type() == UNIQUE_KEYS &&
- new_tablet->enable_unique_key_merge_on_write()) {
- res = _calc_delete_bitmap_for_mow_table(new_tablet,
real_alter_version);
+ if (_new_tablet->keys_type() == UNIQUE_KEYS &&
+ _new_tablet->enable_unique_key_merge_on_write()) {
+ res = _calc_delete_bitmap_for_mow_table(real_alter_version);
if (!res) {
break;
}
} else {
// set state to ready
- std::lock_guard<std::shared_mutex>
new_wlock(new_tablet->get_header_lock());
+ std::lock_guard<std::shared_mutex>
new_wlock(_new_tablet->get_header_lock());
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
- res = new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
+ res = _new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
if (!res) {
break;
}
- new_tablet->save_meta();
+ _new_tablet->save_meta();
}
} while (false);
if (res) {
// _validate_alter_result should be outside the above while loop.
// to avoid requiring the header lock twice.
- res = _validate_alter_result(new_tablet, request);
+ res = _validate_alter_result(request);
}
// if failed convert history data, then just remove the new tablet
if (!res) {
- LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
base_tablet->tablet_id()
- << ", drop new_tablet=" << new_tablet->tablet_id();
+ LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
_base_tablet->tablet_id()
+ << ", drop new_tablet=" << _new_tablet->tablet_id();
// do not drop the new tablet and its data. GC thread will
}
return res;
}
-bool SchemaChangeHandler::tablet_in_converting(int64_t tablet_id) {
+bool SchemaChangeJob::tablet_in_converting(int64_t tablet_id) {
Review Comment:
warning: method 'tablet_in_converting' can be made static
[readability-convert-member-functions-to-static]
be/src/olap/schema_change.h:284:
```diff
- bool tablet_in_converting(int64_t tablet_id);
+ static bool tablet_in_converting(int64_t tablet_id);
```
##########
be/src/olap/schema_change.cpp:
##########
@@ -647,102 +656,119 @@ Status
VSchemaChangeWithSorting::_external_sorting(vector<RowsetSharedPtr>& src_
Merger::Statistics stats;
RETURN_IF_ERROR(Merger::vmerge_rowsets(new_tablet,
ReaderType::READER_ALTER_TABLE,
*new_tablet_schema, rs_readers,
rowset_writer, &stats));
+ LOG_INFO("lightman _external_sorting")
+ .tag("merged_rows", stats.merged_rows)
+ .tag("src_rowsets len", src_rowsets.size());
_add_merged_rows(stats.merged_rows);
_add_filtered_rows(stats.filtered_rows);
return Status::OK();
}
-Status SchemaChangeHandler::process_alter_tablet_v2(const TAlterTabletReqV2&
request) {
+Status VLocalSchemaChangeWithSorting::_inner_process(RowsetReaderSharedPtr
rowset_reader,
+ RowsetWriter*
rowset_writer,
+ BaseTabletSPtr new_tablet,
+ TabletSchemaSPtr
base_tablet_schema,
+ TabletSchemaSPtr
new_tablet_schema) {
+ Defer defer {[&]() {
+ // remove the intermediate rowsets generated by internal sorting
+ for (auto& row_set : _src_rowsets) {
+ _local_storage_engine.add_unused_rowset(row_set);
+ }
+ }};
+ _pending_rs_guards.clear();
+ LOG_INFO("lightman VLocalSchemaChangeWithSorting::_inner_process");
+ return VBaseSchemaChangeWithSorting::_inner_process(rowset_reader,
rowset_writer, new_tablet,
+ base_tablet_schema,
new_tablet_schema);
+}
+
+Status SchemaChangeJob::process_alter_tablet(const TAlterTabletReqV2& request)
{
if (!request.__isset.desc_tbl) {
return Status::Error<INVALID_ARGUMENT>(
"desc_tbl is not set. Maybe the FE version is not equal to the
BE "
"version.");
}
+ if (_base_tablet == nullptr) {
+ return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
+ request.base_tablet_id);
+ }
+ if (_new_tablet == nullptr) {
+ return Status::Error<TABLE_NOT_FOUND>("fail to find new tablet.
new_tablet={}",
+ request.new_tablet_id);
+ }
LOG(INFO) << "begin to do request alter tablet: base_tablet_id=" <<
request.base_tablet_id
<< ", new_tablet_id=" << request.new_tablet_id
<< ", alter_version=" << request.alter_version;
- TabletSharedPtr base_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.base_tablet_id);
- if (base_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
- request.base_tablet_id);
- }
// Lock schema_change_lock util schema change info is stored in tablet
header
- std::unique_lock<std::mutex>
schema_change_lock(base_tablet->get_schema_change_lock(),
+ std::unique_lock<std::mutex>
schema_change_lock(_base_tablet->get_schema_change_lock(),
std::try_to_lock);
if (!schema_change_lock.owns_lock()) {
return Status::Error<TRY_LOCK_FAILED>("failed to obtain schema change
lock. base_tablet={}",
request.base_tablet_id);
}
- Status res = _do_process_alter_tablet_v2(request);
+ Status res = _do_process_alter_tablet(request);
LOG(INFO) << "finished alter tablet process, res=" << res;
return res;
}
-std::shared_mutex SchemaChangeHandler::_mutex;
-std::unordered_set<int64_t> SchemaChangeHandler::_tablet_ids_in_converting;
+SchemaChangeJob::SchemaChangeJob(StorageEngine& local_storage_engine,
+ const TAlterTabletReqV2& request)
+ : _local_storage_engine(local_storage_engine) {
+ _base_tablet =
_local_storage_engine.tablet_manager()->get_tablet(request.base_tablet_id);
+ _new_tablet =
_local_storage_engine.tablet_manager()->get_tablet(request.new_tablet_id);
+ if (_base_tablet && _new_tablet) {
+ _base_tablet_schema = std::make_shared<TabletSchema>();
+
_base_tablet_schema->update_tablet_columns(*_base_tablet->tablet_schema(),
request.columns);
+ // During a schema change, the extracted columns of a variant should
not be included in the tablet schema.
+ // This is because the schema change for a variant needs to ignore the
extracted columns.
+ // Otherwise, the schema types in different rowsets might be
inconsistent. When performing a schema change,
+ // the complete variant is constructed by reading all the sub-columns
of the variant.
+ _new_tablet_schema =
_new_tablet->tablet_schema()->copy_without_extracted_columns();
+ }
+}
// In the past schema change and rollup will create new tablet and will wait
for txns starting before the task to finished
// It will cost a lot of time to wait and the task is very difficult to
understand.
// In alter task v2, FE will call BE to create tablet and send an alter task
to BE to convert historical data.
// The admin should upgrade all BE and then upgrade FE.
// Should delete the old code after upgrade finished.
-Status SchemaChangeHandler::_do_process_alter_tablet_v2(const
TAlterTabletReqV2& request) {
- Status res = Status::OK();
- TabletSharedPtr base_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.base_tablet_id);
- if (base_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
- request.base_tablet_id);
- }
-
- signal::tablet_id = base_tablet->get_table_id();
-
- // new tablet has to exist
- TabletSharedPtr new_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.new_tablet_id);
- if (new_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find new tablet.
new_tablet={}",
- request.new_tablet_id);
- }
+Status SchemaChangeJob::_do_process_alter_tablet(const TAlterTabletReqV2&
request) {
Review Comment:
warning: function '_do_process_alter_tablet' has cognitive complexity of 58
(threshold 50) [readability-function-cognitive-complexity]
```cpp
Status SchemaChangeJob::_do_process_alter_tablet(const TAlterTabletReqV2&
request) {
^
```
<details>
<summary>Additional context</summary>
**be/src/olap/schema_change.cpp:742:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (_new_tablet->tablet_state() != TABLET_NOTREADY) {
^
```
**be/src/olap/schema_change.cpp:763:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (!base_migration_rlock.owns_lock()) {
^
```
**be/src/olap/schema_change.cpp:768:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (!new_migration_rlock.owns_lock()) {
^
```
**be/src/olap/schema_change.cpp:790:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
for (int i = 0; i < num_cols; ++i) {
^
```
**be/src/olap/schema_change.cpp:800:** nesting level increased to 1
```cpp
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
^
```
**be/src/util/trace.h:32:** expanded from macro
'SCOPED_SIMPLE_TRACE_IF_TIMEOUT'
```cpp
SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT(timeout, LOG(WARNING))
^
```
**be/src/util/trace.h:44:** expanded from macro
'SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT'
```cpp
SCOPED_CLEANUP({
\
^
```
**be/src/util/scoped_cleanup.h:33:** expanded from macro 'SCOPED_CLEANUP'
```cpp
auto VARNAME_LINENUM(scoped_cleanup) = MakeScopedCleanup([&] { func_body
});
^
```
**be/src/olap/schema_change.cpp:800:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
^
```
**be/src/util/trace.h:32:** expanded from macro
'SCOPED_SIMPLE_TRACE_IF_TIMEOUT'
```cpp
SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT(timeout, LOG(WARNING))
^
```
**be/src/util/trace.h:49:** expanded from macro
'SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT'
```cpp
if (VARNAME_LINENUM(cost_us) >= VARNAME_LINENUM(timeout_us)) {
\
^
```
**be/src/olap/schema_change.cpp:803:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
do {
^
```
**be/src/olap/schema_change.cpp:806:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (!_get_versions_to_be_changed(&versions_to_be_changed,
&max_rowset)) {
^
```
**be/src/olap/schema_change.cpp:812:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (max_rowset == nullptr || max_rowset->end_version() <
request.alter_version) {
^
```
**be/src/olap/schema_change.cpp:812:** +1
```cpp
if (max_rowset == nullptr || max_rowset->end_version() <
request.alter_version) {
^
```
**be/src/olap/schema_change.cpp:815:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
(max_rowset == nullptr ? 0 :
max_rowset->end_version()),
^
```
**be/src/olap/schema_change.cpp:845:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
RETURN_IF_ERROR(_new_tablet->modify_rowsets(empty_vec,
rowsets_to_delete));
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/olap/schema_change.cpp:845:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
RETURN_IF_ERROR(_new_tablet->modify_rowsets(empty_vec,
rowsets_to_delete));
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/olap/schema_change.cpp:862:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
RETURN_IF_ERROR(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/olap/schema_change.cpp:862:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
RETURN_IF_ERROR(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/olap/schema_change.cpp:864:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (rs_splits.empty()) {
^
```
**be/src/olap/schema_change.cpp:880:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (!res) {
^
```
**be/src/olap/schema_change.cpp:906:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
do {
^
```
**be/src/olap/schema_change.cpp:907:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (!res) {
^
```
**be/src/olap/schema_change.cpp:912:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
RETURN_IF_ERROR(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/olap/schema_change.cpp:912:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
RETURN_IF_ERROR(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/olap/schema_change.cpp:921:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
switch (request.alter_tablet_type) {
^
```
**be/src/olap/schema_change.cpp:932:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (request.__isset.materialized_view_params) {
^
```
**be/src/olap/schema_change.cpp:956:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (!res) {
^
```
**be/src/olap/schema_change.cpp:960:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
if (_new_tablet->keys_type() == UNIQUE_KEYS &&
^
```
**be/src/olap/schema_change.cpp:963:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (!res) {
^
```
**be/src/olap/schema_change.cpp:966:** +1, nesting level increased to 2
```cpp
} else {
^
```
**be/src/olap/schema_change.cpp:969:** nesting level increased to 3
```cpp
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
^
```
**be/src/util/trace.h:32:** expanded from macro
'SCOPED_SIMPLE_TRACE_IF_TIMEOUT'
```cpp
SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT(timeout, LOG(WARNING))
^
```
**be/src/util/trace.h:44:** expanded from macro
'SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT'
```cpp
SCOPED_CLEANUP({
\
^
```
**be/src/util/scoped_cleanup.h:33:** expanded from macro 'SCOPED_CLEANUP'
```cpp
auto VARNAME_LINENUM(scoped_cleanup) = MakeScopedCleanup([&] { func_body
});
^
```
**be/src/olap/schema_change.cpp:969:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
^
```
**be/src/util/trace.h:32:** expanded from macro
'SCOPED_SIMPLE_TRACE_IF_TIMEOUT'
```cpp
SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT(timeout, LOG(WARNING))
^
```
**be/src/util/trace.h:49:** expanded from macro
'SCOPED_SIMPLE_TRACE_TO_STREAM_IF_TIMEOUT'
```cpp
if (VARNAME_LINENUM(cost_us) >= VARNAME_LINENUM(timeout_us)) {
\
^
```
**be/src/olap/schema_change.cpp:971:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (!res) {
^
```
**be/src/olap/schema_change.cpp:978:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (res) {
^
```
**be/src/olap/schema_change.cpp:985:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (!res) {
^
```
</details>
##########
be/src/olap/schema_change.cpp:
##########
@@ -647,102 +656,119 @@
Merger::Statistics stats;
RETURN_IF_ERROR(Merger::vmerge_rowsets(new_tablet,
ReaderType::READER_ALTER_TABLE,
*new_tablet_schema, rs_readers,
rowset_writer, &stats));
+ LOG_INFO("lightman _external_sorting")
+ .tag("merged_rows", stats.merged_rows)
+ .tag("src_rowsets len", src_rowsets.size());
_add_merged_rows(stats.merged_rows);
_add_filtered_rows(stats.filtered_rows);
return Status::OK();
}
-Status SchemaChangeHandler::process_alter_tablet_v2(const TAlterTabletReqV2&
request) {
+Status VLocalSchemaChangeWithSorting::_inner_process(RowsetReaderSharedPtr
rowset_reader,
+ RowsetWriter*
rowset_writer,
+ BaseTabletSPtr new_tablet,
+ TabletSchemaSPtr
base_tablet_schema,
+ TabletSchemaSPtr
new_tablet_schema) {
+ Defer defer {[&]() {
+ // remove the intermediate rowsets generated by internal sorting
+ for (auto& row_set : _src_rowsets) {
+ _local_storage_engine.add_unused_rowset(row_set);
+ }
+ }};
+ _pending_rs_guards.clear();
+ LOG_INFO("lightman VLocalSchemaChangeWithSorting::_inner_process");
+ return VBaseSchemaChangeWithSorting::_inner_process(rowset_reader,
rowset_writer, new_tablet,
+ base_tablet_schema,
new_tablet_schema);
+}
+
+Status SchemaChangeJob::process_alter_tablet(const TAlterTabletReqV2& request)
{
if (!request.__isset.desc_tbl) {
return Status::Error<INVALID_ARGUMENT>(
"desc_tbl is not set. Maybe the FE version is not equal to the
BE "
"version.");
}
+ if (_base_tablet == nullptr) {
+ return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
+ request.base_tablet_id);
+ }
+ if (_new_tablet == nullptr) {
+ return Status::Error<TABLE_NOT_FOUND>("fail to find new tablet.
new_tablet={}",
+ request.new_tablet_id);
+ }
LOG(INFO) << "begin to do request alter tablet: base_tablet_id=" <<
request.base_tablet_id
<< ", new_tablet_id=" << request.new_tablet_id
<< ", alter_version=" << request.alter_version;
- TabletSharedPtr base_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.base_tablet_id);
- if (base_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
- request.base_tablet_id);
- }
// Lock schema_change_lock util schema change info is stored in tablet
header
- std::unique_lock<std::mutex>
schema_change_lock(base_tablet->get_schema_change_lock(),
+ std::unique_lock<std::mutex>
schema_change_lock(_base_tablet->get_schema_change_lock(),
std::try_to_lock);
if (!schema_change_lock.owns_lock()) {
return Status::Error<TRY_LOCK_FAILED>("failed to obtain schema change
lock. base_tablet={}",
request.base_tablet_id);
}
- Status res = _do_process_alter_tablet_v2(request);
+ Status res = _do_process_alter_tablet(request);
LOG(INFO) << "finished alter tablet process, res=" << res;
return res;
}
-std::shared_mutex SchemaChangeHandler::_mutex;
-std::unordered_set<int64_t> SchemaChangeHandler::_tablet_ids_in_converting;
+SchemaChangeJob::SchemaChangeJob(StorageEngine& local_storage_engine,
+ const TAlterTabletReqV2& request)
+ : _local_storage_engine(local_storage_engine) {
+ _base_tablet =
_local_storage_engine.tablet_manager()->get_tablet(request.base_tablet_id);
+ _new_tablet =
_local_storage_engine.tablet_manager()->get_tablet(request.new_tablet_id);
+ if (_base_tablet && _new_tablet) {
+ _base_tablet_schema = std::make_shared<TabletSchema>();
+
_base_tablet_schema->update_tablet_columns(*_base_tablet->tablet_schema(),
request.columns);
+ // During a schema change, the extracted columns of a variant should
not be included in the tablet schema.
+ // This is because the schema change for a variant needs to ignore the
extracted columns.
+ // Otherwise, the schema types in different rowsets might be
inconsistent. When performing a schema change,
+ // the complete variant is constructed by reading all the sub-columns
of the variant.
+ _new_tablet_schema =
_new_tablet->tablet_schema()->copy_without_extracted_columns();
+ }
+}
// In the past schema change and rollup will create new tablet and will wait
for txns starting before the task to finished
// It will cost a lot of time to wait and the task is very difficult to
understand.
// In alter task v2, FE will call BE to create tablet and send an alter task
to BE to convert historical data.
// The admin should upgrade all BE and then upgrade FE.
// Should delete the old code after upgrade finished.
-Status SchemaChangeHandler::_do_process_alter_tablet_v2(const
TAlterTabletReqV2& request) {
- Status res = Status::OK();
- TabletSharedPtr base_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.base_tablet_id);
- if (base_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find base tablet.
base_tablet={}",
- request.base_tablet_id);
- }
-
- signal::tablet_id = base_tablet->get_table_id();
-
- // new tablet has to exist
- TabletSharedPtr new_tablet =
-
ExecEnv::GetInstance()->storage_engine().to_local().tablet_manager()->get_tablet(
- request.new_tablet_id);
- if (new_tablet == nullptr) {
- return Status::Error<TABLE_NOT_FOUND>("fail to find new tablet.
new_tablet={}",
- request.new_tablet_id);
- }
+Status SchemaChangeJob::_do_process_alter_tablet(const TAlterTabletReqV2&
request) {
Review Comment:
warning: function '_do_process_alter_tablet' exceeds recommended
size/complexity thresholds [readability-function-size]
```cpp
Status SchemaChangeJob::_do_process_alter_tablet(const TAlterTabletReqV2&
request) {
^
```
<details>
<summary>Additional context</summary>
**be/src/olap/schema_change.cpp:736:** 256 lines including whitespace and
comments (threshold 80)
```cpp
Status SchemaChangeJob::_do_process_alter_tablet(const TAlterTabletReqV2&
request) {
^
```
</details>
##########
be/src/olap/schema_change.cpp:
##########
@@ -934,80 +944,81 @@
}
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.insert(new_tablet->tablet_id());
+ _tablet_ids_in_converting.insert(_new_tablet->tablet_id());
}
int64_t real_alter_version = 0;
+ sc_params.enable_unique_key_merge_on_write =
+ _new_tablet->enable_unique_key_merge_on_write();
res = _convert_historical_rowsets(sc_params, &real_alter_version);
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.erase(new_tablet->tablet_id());
+ _tablet_ids_in_converting.erase(_new_tablet->tablet_id());
}
if (!res) {
break;
}
- if (new_tablet->keys_type() == UNIQUE_KEYS &&
- new_tablet->enable_unique_key_merge_on_write()) {
- res = _calc_delete_bitmap_for_mow_table(new_tablet,
real_alter_version);
+ if (_new_tablet->keys_type() == UNIQUE_KEYS &&
+ _new_tablet->enable_unique_key_merge_on_write()) {
+ res = _calc_delete_bitmap_for_mow_table(real_alter_version);
if (!res) {
break;
}
} else {
// set state to ready
- std::lock_guard<std::shared_mutex>
new_wlock(new_tablet->get_header_lock());
+ std::lock_guard<std::shared_mutex>
new_wlock(_new_tablet->get_header_lock());
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
- res = new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
+ res = _new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
if (!res) {
break;
}
- new_tablet->save_meta();
+ _new_tablet->save_meta();
}
} while (false);
if (res) {
// _validate_alter_result should be outside the above while loop.
// to avoid requiring the header lock twice.
- res = _validate_alter_result(new_tablet, request);
+ res = _validate_alter_result(request);
}
// if failed convert history data, then just remove the new tablet
if (!res) {
- LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
base_tablet->tablet_id()
- << ", drop new_tablet=" << new_tablet->tablet_id();
+ LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
_base_tablet->tablet_id()
+ << ", drop new_tablet=" << _new_tablet->tablet_id();
// do not drop the new tablet and its data. GC thread will
}
return res;
}
-bool SchemaChangeHandler::tablet_in_converting(int64_t tablet_id) {
+bool SchemaChangeJob::tablet_in_converting(int64_t tablet_id) {
std::shared_lock rdlock(_mutex);
return _tablet_ids_in_converting.find(tablet_id) !=
_tablet_ids_in_converting.end();
}
-Status SchemaChangeHandler::_get_versions_to_be_changed(
- TabletSharedPtr base_tablet, std::vector<Version>*
versions_to_be_changed,
- RowsetSharedPtr* max_rowset) {
- RowsetSharedPtr rowset = base_tablet->get_rowset_with_max_version();
+Status SchemaChangeJob::_get_versions_to_be_changed(std::vector<Version>*
versions_to_be_changed,
+ RowsetSharedPtr*
max_rowset) {
+ RowsetSharedPtr rowset = _base_tablet->get_rowset_with_max_version();
if (rowset == nullptr) {
return Status::Error<ALTER_DELTA_DOES_NOT_EXISTS>("Tablet has no
version. base_tablet={}",
-
base_tablet->tablet_id());
+
_base_tablet->tablet_id());
}
*max_rowset = rowset;
- RETURN_IF_ERROR(base_tablet->capture_consistent_versions_unlocked(
+ RETURN_IF_ERROR(_base_tablet->capture_consistent_versions_unlocked(
Version(0, rowset->version().second), versions_to_be_changed,
false, false));
return Status::OK();
}
// The `real_alter_version` parameter indicates that the version of
[0-real_alter_version] is
// converted from a base tablet, only used for the mow table now.
-Status SchemaChangeHandler::_convert_historical_rowsets(const
SchemaChangeParams& sc_params,
- int64_t*
real_alter_version) {
+Status SchemaChangeJob::_convert_historical_rowsets(const SchemaChangeParams&
sc_params,
Review Comment:
warning: method '_convert_historical_rowsets' can be made static
[readability-convert-member-functions-to-static]
be/src/olap/schema_change.h:313:
```diff
- Status _convert_historical_rowsets(const SchemaChangeParams& sc_params,
+ static Status _convert_historical_rowsets(const SchemaChangeParams&
sc_params,
```
##########
be/src/olap/schema_change.cpp:
##########
@@ -1144,19 +1152,16 @@
// @static
// Analyze the mapping of the column and the mapping of the filter key
-Status SchemaChangeHandler::_parse_request(const SchemaChangeParams& sc_params,
- BlockChanger* changer, bool*
sc_sorting,
- bool* sc_directly) {
+Status SchemaChangeJob::parse_request(const SchemaChangeParams& sc_params,
Review Comment:
warning: function 'parse_request' exceeds recommended size/complexity
thresholds [readability-function-size]
```cpp
Status SchemaChangeJob::parse_request(const SchemaChangeParams& sc_params,
^
```
<details>
<summary>Additional context</summary>
**be/src/olap/schema_change.cpp:1154:** 150 lines including whitespace and
comments (threshold 80)
```cpp
Status SchemaChangeJob::parse_request(const SchemaChangeParams& sc_params,
^
```
</details>
##########
be/src/olap/schema_change.cpp:
##########
@@ -1321,11 +1326,10 @@
return Status::OK();
}
-Status SchemaChangeHandler::_validate_alter_result(TabletSharedPtr new_tablet,
- const TAlterTabletReqV2&
request) {
+Status SchemaChangeJob::_validate_alter_result(const TAlterTabletReqV2&
request) {
Review Comment:
warning: method '_validate_alter_result' can be made static
[readability-convert-member-functions-to-static]
be/src/olap/schema_change.h:311:
```diff
- Status _validate_alter_result(const TAlterTabletReqV2& request);
+ static Status _validate_alter_result(const TAlterTabletReqV2& request);
```
##########
be/src/olap/schema_change.cpp:
##########
@@ -1358,57 +1362,70 @@
// incremental rowsets.
// 4. Switch the tablet status to TABLET_RUNNING. The newly imported
// data will calculate delete bitmap.
-Status SchemaChangeHandler::_calc_delete_bitmap_for_mow_table(TabletSharedPtr
new_tablet,
- int64_t
alter_version) {
-
DBUG_EXECUTE_IF("SchemaChangeHandler._calc_delete_bitmap_for_mow_table.random_failed",
{
+Status SchemaChangeJob::_calc_delete_bitmap_for_mow_table(int64_t
alter_version) {
Review Comment:
warning: method '_calc_delete_bitmap_for_mow_table' can be made static
[readability-convert-member-functions-to-static]
be/src/olap/schema_change.h:320:
```diff
- Status _calc_delete_bitmap_for_mow_table(int64_t alter_version);
+ static Status _calc_delete_bitmap_for_mow_table(int64_t alter_version);
```
##########
be/src/olap/schema_change.cpp:
##########
@@ -934,80 +944,81 @@
}
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.insert(new_tablet->tablet_id());
+ _tablet_ids_in_converting.insert(_new_tablet->tablet_id());
}
int64_t real_alter_version = 0;
+ sc_params.enable_unique_key_merge_on_write =
+ _new_tablet->enable_unique_key_merge_on_write();
res = _convert_historical_rowsets(sc_params, &real_alter_version);
{
std::lock_guard<std::shared_mutex> wrlock(_mutex);
- _tablet_ids_in_converting.erase(new_tablet->tablet_id());
+ _tablet_ids_in_converting.erase(_new_tablet->tablet_id());
}
if (!res) {
break;
}
- if (new_tablet->keys_type() == UNIQUE_KEYS &&
- new_tablet->enable_unique_key_merge_on_write()) {
- res = _calc_delete_bitmap_for_mow_table(new_tablet,
real_alter_version);
+ if (_new_tablet->keys_type() == UNIQUE_KEYS &&
+ _new_tablet->enable_unique_key_merge_on_write()) {
+ res = _calc_delete_bitmap_for_mow_table(real_alter_version);
if (!res) {
break;
}
} else {
// set state to ready
- std::lock_guard<std::shared_mutex>
new_wlock(new_tablet->get_header_lock());
+ std::lock_guard<std::shared_mutex>
new_wlock(_new_tablet->get_header_lock());
SCOPED_SIMPLE_TRACE_IF_TIMEOUT(TRACE_TABLET_LOCK_THRESHOLD);
- res = new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
+ res = _new_tablet->set_tablet_state(TabletState::TABLET_RUNNING);
if (!res) {
break;
}
- new_tablet->save_meta();
+ _new_tablet->save_meta();
}
} while (false);
if (res) {
// _validate_alter_result should be outside the above while loop.
// to avoid requiring the header lock twice.
- res = _validate_alter_result(new_tablet, request);
+ res = _validate_alter_result(request);
}
// if failed convert history data, then just remove the new tablet
if (!res) {
- LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
base_tablet->tablet_id()
- << ", drop new_tablet=" << new_tablet->tablet_id();
+ LOG(WARNING) << "failed to alter tablet. base_tablet=" <<
_base_tablet->tablet_id()
+ << ", drop new_tablet=" << _new_tablet->tablet_id();
// do not drop the new tablet and its data. GC thread will
}
return res;
}
-bool SchemaChangeHandler::tablet_in_converting(int64_t tablet_id) {
+bool SchemaChangeJob::tablet_in_converting(int64_t tablet_id) {
std::shared_lock rdlock(_mutex);
return _tablet_ids_in_converting.find(tablet_id) !=
_tablet_ids_in_converting.end();
}
-Status SchemaChangeHandler::_get_versions_to_be_changed(
- TabletSharedPtr base_tablet, std::vector<Version>*
versions_to_be_changed,
- RowsetSharedPtr* max_rowset) {
- RowsetSharedPtr rowset = base_tablet->get_rowset_with_max_version();
+Status SchemaChangeJob::_get_versions_to_be_changed(std::vector<Version>*
versions_to_be_changed,
+ RowsetSharedPtr*
max_rowset) {
+ RowsetSharedPtr rowset = _base_tablet->get_rowset_with_max_version();
if (rowset == nullptr) {
return Status::Error<ALTER_DELTA_DOES_NOT_EXISTS>("Tablet has no
version. base_tablet={}",
-
base_tablet->tablet_id());
+
_base_tablet->tablet_id());
}
*max_rowset = rowset;
- RETURN_IF_ERROR(base_tablet->capture_consistent_versions_unlocked(
+ RETURN_IF_ERROR(_base_tablet->capture_consistent_versions_unlocked(
Version(0, rowset->version().second), versions_to_be_changed,
false, false));
return Status::OK();
}
// The `real_alter_version` parameter indicates that the version of
[0-real_alter_version] is
// converted from a base tablet, only used for the mow table now.
-Status SchemaChangeHandler::_convert_historical_rowsets(const
SchemaChangeParams& sc_params,
- int64_t*
real_alter_version) {
+Status SchemaChangeJob::_convert_historical_rowsets(const SchemaChangeParams&
sc_params,
Review Comment:
warning: function '_convert_historical_rowsets' exceeds recommended
size/complexity thresholds [readability-function-size]
```cpp
Status SchemaChangeJob::_convert_historical_rowsets(const
SchemaChangeParams& sc_params,
^
```
<details>
<summary>Additional context</summary>
**be/src/olap/schema_change.cpp:1016:** 131 lines including whitespace and
comments (threshold 80)
```cpp
Status SchemaChangeJob::_convert_historical_rowsets(const
SchemaChangeParams& sc_params,
^
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
