github-actions[bot] commented on code in PR #31716:
URL: https://github.com/apache/doris/pull/31716#discussion_r1511016323
##########
be/src/io/fs/s3_file_writer.cpp:
##########
@@ -250,7 +241,10 @@ Status S3FileWriter::close() {
}
Status S3FileWriter::appendv(const Slice* data, size_t data_cnt) {
Review Comment:
warning: function 'appendv' has cognitive complexity of 65 (threshold 50)
[readability-function-cognitive-complexity]
```cpp
Status S3FileWriter::appendv(const Slice* data, size_t data_cnt) {
^
```
<details>
<summary>Additional context</summary>
**be/src/io/fs/s3_file_writer.cpp:243:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (closed()) [[unlikely]] {
^
```
**be/src/io/fs/s3_file_writer.cpp:248:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
DBUG_EXECUTE_IF("s3_file_writer::appendv",
^
```
**be/src/util/debug_points.h:34:** expanded from macro 'DBUG_EXECUTE_IF'
```cpp
if (UNLIKELY(config::enable_debug_points)) {
\
^
```
**be/src/io/fs/s3_file_writer.cpp:248:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
DBUG_EXECUTE_IF("s3_file_writer::appendv",
^
```
**be/src/util/debug_points.h:36:** expanded from macro 'DBUG_EXECUTE_IF'
```cpp
if (dp) {
\
^
```
**be/src/io/fs/s3_file_writer.cpp:250:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
for (size_t i = 0; i < data_cnt; i++) {
^
```
**be/src/io/fs/s3_file_writer.cpp:252:** +2, including nesting penalty of 1,
nesting level increased to 2
```cpp
for (size_t pos = 0, data_size_to_append = 0; pos < data_size; pos
+= data_size_to_append) {
^
```
**be/src/io/fs/s3_file_writer.cpp:253:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (_failed) {
^
```
**be/src/io/fs/s3_file_writer.cpp:256:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (!_pending_buf) {
^
```
**be/src/io/fs/s3_file_writer.cpp:260:** nesting level increased to 4
```cpp
[part_num = _cur_part_num,
this](UploadFileBuffer& buf) {
^
```
**be/src/io/fs/s3_file_writer.cpp:265:** nesting level increased to 4
```cpp
.set_sync_after_complete_task([this, part_num =
_cur_part_num](Status s) {
^
```
**be/src/io/fs/s3_file_writer.cpp:267:** +5, including nesting penalty of 4,
nesting level increased to 5
```cpp
if (!s.ok()) [[unlikely]] {
^
```
**be/src/io/fs/s3_file_writer.cpp:281:** nesting level increased to 4
```cpp
.set_is_cancelled([this]() { return _failed.load();
});
^
```
**be/src/io/fs/s3_file_writer.cpp:282:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
if (_write_file_cache) {
^
```
**be/src/io/fs/s3_file_writer.cpp:299:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
RETURN_IF_ERROR(builder.build(&_pending_buf));
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/io/fs/s3_file_writer.cpp:299:** +5, including nesting penalty of 4,
nesting level increased to 5
```cpp
RETURN_IF_ERROR(builder.build(&_pending_buf));
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/io/fs/s3_file_writer.cpp:308:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
RETURN_IF_ERROR(_pending_buf->append_data(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/io/fs/s3_file_writer.cpp:308:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
RETURN_IF_ERROR(_pending_buf->append_data(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/io/fs/s3_file_writer.cpp:314:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (_pending_buf->get_size() == buffer_size) {
^
```
**be/src/io/fs/s3_file_writer.cpp:317:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
if (_cur_part_num == 1) {
^
```
**be/src/io/fs/s3_file_writer.cpp:318:** +5, including nesting penalty of 4,
nesting level increased to 5
```cpp
RETURN_IF_ERROR(_create_multi_upload_request());
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/io/fs/s3_file_writer.cpp:318:** +6, including nesting penalty of 5,
nesting level increased to 6
```cpp
RETURN_IF_ERROR(_create_multi_upload_request());
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/io/fs/s3_file_writer.cpp:322:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
RETURN_IF_ERROR(_pending_buf->submit(std::move(_pending_buf)));
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/io/fs/s3_file_writer.cpp:322:** +5, including nesting penalty of 4,
nesting level increased to 5
```cpp
RETURN_IF_ERROR(_pending_buf->submit(std::move(_pending_buf)));
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
</details>
##########
be/src/cloud/cloud_meta_mgr.cpp:
##########
@@ -317,6 +316,99 @@ static Status retry_rpc(std::string_view op_name, const
Request& req, Response*
return Status::RpcError("failed to {}: rpc timeout, last msg={}", op_name,
error_msg);
}
+Status sync_tablet_delete_bitmap(CloudTablet* tablet, int64_t old_max_version,
+ std::ranges::range auto&& rs_metas, const
TabletStatsPB& stats,
+ const TabletIndexPB& idx, DeleteBitmap*
delete_bitmap) {
+ if (rs_metas.empty()) {
+ return Status::OK();
+ }
+
+ std::shared_ptr<MetaService_Stub> stub;
+ RETURN_IF_ERROR(MetaServiceProxy::get_client(&stub));
+
+ int64_t new_max_version = std::max(old_max_version,
rs_metas.rbegin()->end_version());
+ brpc::Controller cntl;
+ // When there are many delete bitmaps that need to be synchronized, it
+ // may take a longer time, especially when loading the tablet for the
+ // first time, so set a relatively long timeout time.
+ cntl.set_timeout_ms(3 * config::meta_service_brpc_timeout_ms);
+ GetDeleteBitmapRequest req;
+ GetDeleteBitmapResponse res;
+ req.set_cloud_unique_id(config::cloud_unique_id);
+ req.set_tablet_id(tablet->tablet_id());
+ req.set_base_compaction_cnt(stats.base_compaction_cnt());
+ req.set_cumulative_compaction_cnt(stats.cumulative_compaction_cnt());
+ req.set_cumulative_point(stats.cumulative_point());
+ *(req.mutable_idx()) = idx;
+ // New rowset sync all versions of delete bitmap
+ for (const auto& rs_meta : rs_metas) {
+ req.add_rowset_ids(rs_meta.rowset_id_v2());
+ req.add_begin_versions(0);
+ req.add_end_versions(new_max_version);
+ }
+
+ // old rowset sync incremental versions of delete bitmap
+ if (old_max_version > 0 && old_max_version < new_max_version) {
+ RowsetIdUnorderedSet all_rs_ids;
+ RETURN_IF_ERROR(tablet->get_all_rs_id(old_max_version, &all_rs_ids));
+ for (const auto& rs_id : all_rs_ids) {
+ req.add_rowset_ids(rs_id.to_string());
+ req.add_begin_versions(old_max_version + 1);
+ req.add_end_versions(new_max_version);
+ }
+ }
+
+ VLOG_DEBUG << "send GetDeleteBitmapRequest: " << req.ShortDebugString();
+ stub->get_delete_bitmap(&cntl, &req, &res, nullptr);
+ if (cntl.Failed()) {
+ return Status::RpcError("failed to get delete bitmap: {}",
cntl.ErrorText());
+ }
+ if (res.status().code() == MetaServiceCode::TABLET_NOT_FOUND) {
+ return Status::NotFound("failed to get delete bitmap: {}",
res.status().msg());
+ }
+ // The delete bitmap of stale rowsets will be removed when commit
compaction job,
+ // then delete bitmap of stale rowsets cannot be obtained. But the rowsets
obtained
+ // by sync_tablet_rowsets may include these stale rowsets. When this case
happend, the
+ // error code of ROWSETS_EXPIRED will be returned, we need to retry sync
rowsets again.
+ //
+ // Be query thread meta-service Be compaction thread
+ // | | |
+ // | get rowset | |
+ // |--------------------------->| |
+ // | return get rowset | |
+ // |<---------------------------| |
+ // | | commit job |
+ // | |<------------------------|
+ // | | return commit job |
+ // | |------------------------>|
+ // | get delete bitmap | |
+ // |--------------------------->| |
+ // | return get delete bitmap | |
+ // |<---------------------------| |
+ // | | |
+ if (res.status().code() == MetaServiceCode::ROWSETS_EXPIRED) {
+ return Status::Error<ErrorCode::ROWSETS_EXPIRED, false>("failed to get
delete bitmap: {}",
+
res.status().msg());
+ }
+ if (res.status().code() != MetaServiceCode::OK) {
+ return Status::Error<ErrorCode::INTERNAL_ERROR, false>("failed to get
delete bitmap: {}",
+
res.status().msg());
+ }
+ const auto& rowset_ids = res.rowset_ids();
+ const auto& segment_ids = res.segment_ids();
+ const auto& vers = res.versions();
+ const auto& delete_bitmaps = res.segment_delete_bitmaps();
+ for (size_t i = 0; i < rowset_ids.size(); i++) {
+ RowsetId rst_id;
+ rst_id.init(rowset_ids[i]);
+ delete_bitmap->merge({rst_id, segment_ids[i], vers[i]},
+ roaring::Roaring::read(delete_bitmaps[i].data()));
+ }
+ return Status::OK();
+}
+
+} // namespace
+
Review Comment:
warning: method 'get_tablet_meta' can be made static
[readability-convert-member-functions-to-static]
be/src/cloud/cloud_meta_mgr.h:53:
```diff
- Status get_tablet_meta(int64_t tablet_id, std::shared_ptr<TabletMeta>*
tablet_meta);
+ static Status get_tablet_meta(int64_t tablet_id,
std::shared_ptr<TabletMeta>* tablet_meta);
```
##########
be/src/io/hdfs_util.cpp:
##########
@@ -0,0 +1,143 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "io/hdfs_util.h"
+
+#include <ostream>
+
+#include "common/logging.h"
+#include "gutil/hash/hash.h"
+#include "io/fs/err_utils.h"
+#include "io/hdfs_builder.h"
+
+namespace doris::io {
+namespace {
+
+Status create_hdfs_fs(const THdfsParams& hdfs_params, hdfsFS* fs) {
+ HDFSCommonBuilder builder;
+ RETURN_IF_ERROR(create_hdfs_builder(hdfs_params, hdfs_params.fs_name,
&builder));
+ hdfsFS hdfs_fs = hdfsBuilderConnect(builder.get());
+ if (hdfs_fs == nullptr) {
+ return Status::IOError("failed to connect to hdfs {}: {}",
hdfs_params.fs_name,
+ hdfs_error());
+ }
+ *fs = hdfs_fs;
+ return Status::OK();
+}
+
+uint64 hdfs_hash_code(const THdfsParams& hdfs_params) {
+ uint64 hash_code = 0;
+ hash_code ^= Fingerprint(hdfs_params.fs_name);
+ if (hdfs_params.__isset.user) {
+ hash_code ^= Fingerprint(hdfs_params.user);
+ }
+ if (hdfs_params.__isset.hdfs_kerberos_principal) {
+ hash_code ^= Fingerprint(hdfs_params.hdfs_kerberos_principal);
+ }
+ if (hdfs_params.__isset.hdfs_kerberos_keytab) {
+ hash_code ^= Fingerprint(hdfs_params.hdfs_kerberos_keytab);
+ }
+ if (hdfs_params.__isset.hdfs_conf) {
+ std::map<std::string, std::string> conf_map;
+ for (const auto& conf : hdfs_params.hdfs_conf) {
+ conf_map[conf.key] = conf.value;
+ }
+ for (auto& conf : conf_map) {
+ hash_code ^= Fingerprint(conf.first);
+ hash_code ^= Fingerprint(conf.second);
+ }
+ }
+ return hash_code;
+}
+
+} // namespace
+
+void HdfsHandlerCache::_clean_invalid() {
Review Comment:
warning: method '_clean_invalid' can be made static
[readability-convert-member-functions-to-static]
be/src/io/hdfs_util.h:113:
```diff
- void _clean_invalid();
+ static void _clean_invalid();
```
##########
be/src/io/hdfs_util.cpp:
##########
@@ -0,0 +1,143 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "io/hdfs_util.h"
+
+#include <ostream>
+
+#include "common/logging.h"
+#include "gutil/hash/hash.h"
+#include "io/fs/err_utils.h"
+#include "io/hdfs_builder.h"
+
+namespace doris::io {
+namespace {
+
+Status create_hdfs_fs(const THdfsParams& hdfs_params, hdfsFS* fs) {
+ HDFSCommonBuilder builder;
+ RETURN_IF_ERROR(create_hdfs_builder(hdfs_params, hdfs_params.fs_name,
&builder));
+ hdfsFS hdfs_fs = hdfsBuilderConnect(builder.get());
+ if (hdfs_fs == nullptr) {
+ return Status::IOError("failed to connect to hdfs {}: {}",
hdfs_params.fs_name,
+ hdfs_error());
+ }
+ *fs = hdfs_fs;
+ return Status::OK();
+}
+
+uint64 hdfs_hash_code(const THdfsParams& hdfs_params) {
+ uint64 hash_code = 0;
+ hash_code ^= Fingerprint(hdfs_params.fs_name);
+ if (hdfs_params.__isset.user) {
+ hash_code ^= Fingerprint(hdfs_params.user);
+ }
+ if (hdfs_params.__isset.hdfs_kerberos_principal) {
+ hash_code ^= Fingerprint(hdfs_params.hdfs_kerberos_principal);
+ }
+ if (hdfs_params.__isset.hdfs_kerberos_keytab) {
+ hash_code ^= Fingerprint(hdfs_params.hdfs_kerberos_keytab);
+ }
+ if (hdfs_params.__isset.hdfs_conf) {
+ std::map<std::string, std::string> conf_map;
+ for (const auto& conf : hdfs_params.hdfs_conf) {
+ conf_map[conf.key] = conf.value;
+ }
+ for (auto& conf : conf_map) {
+ hash_code ^= Fingerprint(conf.first);
+ hash_code ^= Fingerprint(conf.second);
+ }
+ }
+ return hash_code;
+}
+
+} // namespace
+
+void HdfsHandlerCache::_clean_invalid() {
+ std::vector<uint64> removed_handle;
+ for (auto& item : _cache) {
+ if (item.second->invalid() && item.second->ref_cnt() == 0) {
+ removed_handle.emplace_back(item.first);
+ }
+ }
+ for (auto& handle : removed_handle) {
+ _cache.erase(handle);
+ }
+}
+
+void HdfsHandlerCache::_clean_oldest() {
Review Comment:
warning: method '_clean_oldest' can be made static
[readability-convert-member-functions-to-static]
be/src/io/hdfs_util.h:114:
```diff
- void _clean_oldest();
+ static void _clean_oldest();
```
--
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]
