github-actions[bot] commented on code in PR #31716:
URL: https://github.com/apache/doris/pull/31716#discussion_r1510564256
##########
be/src/cloud/cloud_meta_mgr.cpp:
##########
@@ -714,7 +712,8 @@
return retry_rpc("precommit txn", req, &res,
&MetaService_Stub::precommit_txn);
}
-Status CloudMetaMgr::get_s3_info(std::vector<std::tuple<std::string, S3Conf>>*
s3_infos) {
+Status CloudMetaMgr::get_s3_info(
Review Comment:
warning: method 'get_s3_info' can be made static
[readability-convert-member-functions-to-static]
```suggestion
static Status CloudMetaMgr::get_s3_info(
```
##########
be/src/io/fs/broker_file_reader.h:
##########
@@ -19,7 +19,6 @@
#include <gen_cpp/PaloBrokerService_types.h>
Review Comment:
warning: 'gen_cpp/PaloBrokerService_types.h' file not found
[clang-diagnostic-error]
```cpp
#include <gen_cpp/PaloBrokerService_types.h>
^
```
##########
be/src/cloud/cloud_meta_mgr.cpp:
##########
@@ -318,6 +315,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,
Review Comment:
warning: function 'sync_tablet_delete_bitmap' exceeds recommended
size/complexity thresholds [readability-function-size]
```cpp
Status sync_tablet_delete_bitmap(CloudTablet* tablet, int64_t
old_max_version,
^
```
<details>
<summary>Additional context</summary>
**be/src/cloud/cloud_meta_mgr.cpp:317:** 87 lines including whitespace and
comments (threshold 80)
```cpp
Status sync_tablet_delete_bitmap(CloudTablet* tablet, int64_t
old_max_version,
^
```
</details>
##########
be/src/io/fs/local_file_writer.h:
##########
@@ -29,21 +28,27 @@ namespace doris::io {
class LocalFileWriter final : public FileWriter {
public:
- LocalFileWriter(Path path, int fd, FileSystemSPtr fs, bool sync_data =
true);
- LocalFileWriter(Path path, int fd);
+ LocalFileWriter(Path path, int fd, bool sync_data = true);
~LocalFileWriter() override;
Status close() override;
Status appendv(const Slice* data, size_t data_cnt) override;
Status finalize() override;
+ const Path& path() const override { return _path; }
+ size_t bytes_appended() const override;
+ bool closed() const override { return _closed; }
private:
void _abort();
Status _close(bool sync);
+private:
Review Comment:
warning: redundant access specifier has the same accessibility as the
previous access specifier [readability-redundant-access-specifiers]
```suggestion
```
<details>
<summary>Additional context</summary>
**be/src/io/fs/local_file_writer.h:40:** previously declared here
```cpp
private:
^
```
</details>
##########
be/src/io/hdfs_util.cpp:
##########
@@ -0,0 +1,142 @@
+// 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 "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:108:
```diff
- void _clean_oldest();
+ static void _clean_oldest();
```
##########
be/src/io/fs/broker_file_writer.h:
##########
@@ -19,14 +19,11 @@
#include <gen_cpp/PaloBrokerService_types.h>
Review Comment:
warning: 'gen_cpp/PaloBrokerService_types.h' file not found
[clang-diagnostic-error]
```cpp
#include <gen_cpp/PaloBrokerService_types.h>
^
```
##########
be/src/io/fs/s3_file_system.h:
##########
@@ -39,9 +37,36 @@ namespace Aws::Utils::Threading {
class PooledThreadExecutor;
} // namespace Aws::Utils::Threading
-namespace doris {
-namespace io {
-struct FileInfo;
+namespace doris::io {
+
+// In runtime, AK and SK may be modified, and the original `S3Client` instance
will be replaced.
+// The `S3FileReader` cached by the `Segment` must hold a shared
`S3ClientHolder` in order to
+// access S3 data with latest AK SK.
+class S3ClientHolder {
+public:
+ explicit S3ClientHolder(S3ClientConf conf);
+ ~S3ClientHolder();
+
+ Status init();
+
+ // Update s3 conf and reset client if `conf` is different. This method is
threadsafe.
+ Status reset(const S3ClientConf& conf);
+
+ std::shared_ptr<Aws::S3::S3Client> get() const {
Review Comment:
warning: method 'get' can be made static
[readability-convert-member-functions-to-static]
```suggestion
static std::shared_ptr<Aws::S3::S3Client> get() {
```
##########
be/src/util/s3_util.cpp:
##########
@@ -125,22 +138,7 @@ S3ClientFactory& S3ClientFactory::instance() {
return ret;
}
-bool S3ClientFactory::is_s3_conf_valid(const std::map<std::string,
std::string>& prop) {
- StringCaseMap<std::string> properties(prop.begin(), prop.end());
- if (properties.find(S3_ENDPOINT) == properties.end() ||
- properties.find(S3_REGION) == properties.end()) {
- DCHECK(false) << "aws properties is incorrect.";
- LOG(ERROR) << "aws properties is incorrect.";
- return false;
- }
- return true;
-}
-
-bool S3ClientFactory::is_s3_conf_valid(const S3Conf& s3_conf) {
- return !s3_conf.endpoint.empty();
-}
-
-std::shared_ptr<Aws::S3::S3Client> S3ClientFactory::create(const S3Conf&
s3_conf) {
+std::shared_ptr<Aws::S3::S3Client> S3ClientFactory::create(const S3ClientConf&
s3_conf) {
Review Comment:
warning: method 'create' can be made static
[readability-convert-member-functions-to-static]
be/src/util/s3_util.h:124:
```diff
- std::shared_ptr<Aws::S3::S3Client> create(const S3ClientConf& s3_conf);
+ static std::shared_ptr<Aws::S3::S3Client> create(const S3ClientConf&
s3_conf);
```
##########
be/src/io/hdfs_util.cpp:
##########
@@ -0,0 +1,142 @@
+// 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 "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:107:
```diff
- void _clean_invalid();
+ static void _clean_invalid();
```
##########
be/src/io/fs/broker_file_system.cpp:
##########
@@ -68,36 +69,38 @@ inline const std::string& client_id(const TNetworkAddress&
addr) {
#endif
#ifndef CHECK_BROKER_CLIENT
-#define CHECK_BROKER_CLIENT(client) \
- if (!client || !client->is_alive()) { \
- return Status::IOError("connect to broker failed"); \
+#define CHECK_BROKER_CLIENT(client) \
+ if (!client || !client->is_alive()) { \
+ return Status::InternalError("connect to broker failed"); \
}
#endif
-Status BrokerFileSystem::create(const TNetworkAddress& broker_addr,
- const std::map<std::string, std::string>&
broker_prop,
- std::shared_ptr<BrokerFileSystem>* fs) {
- (*fs).reset(new BrokerFileSystem(broker_addr, broker_prop));
- return (*fs)->connect();
+Result<std::shared_ptr<BrokerFileSystem>> BrokerFileSystem::create(
+ const TNetworkAddress& broker_addr, const std::map<std::string,
std::string>& broker_prop,
+ int64_t id) {
+ std::shared_ptr<BrokerFileSystem> fs(new BrokerFileSystem(broker_addr,
broker_prop, id));
+ RETURN_IF_ERROR_RESULT(fs->init());
+ return fs;
}
BrokerFileSystem::BrokerFileSystem(const TNetworkAddress& broker_addr,
- const std::map<std::string, std::string>&
broker_prop)
- : RemoteFileSystem("", "", FileSystemType::BROKER),
+ const std::map<std::string, std::string>&
broker_prop,
+ int64_t id)
+ : RemoteFileSystem("", id, FileSystemType::BROKER),
_broker_addr(broker_addr),
_broker_prop(broker_prop) {}
-Status BrokerFileSystem::connect_impl() {
+Status BrokerFileSystem::init() {
Review Comment:
warning: method 'init' can be made static
[readability-convert-member-functions-to-static]
be/src/io/fs/broker_file_system.h:70:
```diff
- Status init();
+ static Status init();
```
##########
be/src/io/fs/s3_file_writer.cpp:
##########
@@ -250,7 +247,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:249:** +1, including nesting penalty of 0,
nesting level increased to 1
```cpp
if (closed()) [[unlikely]] {
^
```
**be/src/io/fs/s3_file_writer.cpp:254:** +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:254:** +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:256:** +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:258:** +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:259:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (_failed) {
^
```
**be/src/io/fs/s3_file_writer.cpp:262:** +3, including nesting penalty of 2,
nesting level increased to 3
```cpp
if (!_pending_buf) {
^
```
**be/src/io/fs/s3_file_writer.cpp:266:** nesting level increased to 4
```cpp
[part_num = _cur_part_num,
this](UploadFileBuffer& buf) {
^
```
**be/src/io/fs/s3_file_writer.cpp:271:** 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:273:** +5, including nesting penalty of 4,
nesting level increased to 5
```cpp
if (!s.ok()) [[unlikely]] {
^
```
**be/src/io/fs/s3_file_writer.cpp:287:** nesting level increased to 4
```cpp
.set_is_cancelled([this]() { return _failed.load();
});
^
```
**be/src/io/fs/s3_file_writer.cpp:288:** +4, including nesting penalty of 3,
nesting level increased to 4
```cpp
if (_write_file_cache) {
^
```
**be/src/io/fs/s3_file_writer.cpp:305:** +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:305:** +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:314:** +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:314:** +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:320:** +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:323:** +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:324:** +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:324:** +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:328:** +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:328:** +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>
--
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]
