github-actions[bot] commented on code in PR #18009:
URL: https://github.com/apache/doris/pull/18009#discussion_r1143787061
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -181,6 +194,212 @@ Status LocalFileSystem::link_file_impl(const Path& src,
const Path& dest) {
return Status::OK();
}
+Status LocalFileSystem::canonicalize(const Path& path, std::string* real_path)
{
+ std::error_code ec;
+ Path res = std::filesystem::canonical(path, ec);
+ if (ec) {
+ return Status::IOError("failed to canonicalize path {}: {}",
path.native(),
+ errcode_to_str(ec));
+ }
+ *real_path = res.string();
+ return Status::OK();
+}
+
+Status LocalFileSystem::is_directory(const Path& path, bool* res) {
+ auto tmp_path = absolute_path(path);
+ std::error_code ec;
+ *res = std::filesystem::is_directory(tmp_path, ec);
+ if (ec) {
+ return Status::IOError("failed to check is dir {}: {}",
tmp_path.native(),
+ errcode_to_str(ec));
+ }
+ return Status::OK();
+}
+
+Status LocalFileSystem::md5sum(const Path& file, std::string* md5sum) {
+ auto path = absolute_path(file);
+ if (bthread_self() == 0) {
+ return md5sum_impl(path, md5sum);
+ }
+ Status s;
+ auto task = [&] { s = md5sum_impl(path, md5sum); };
+ AsyncIO::run_task(task, _type);
+ return s;
+}
+
+Status LocalFileSystem::md5sum_impl(const Path& file, std::string* md5sum) {
+ int fd = open(file.c_str(), O_RDONLY);
+ if (fd < 0) {
+ return Status::IOError("failed to open file for md5sum {}: {}",
file.native(),
+ errno_to_str());
+ }
+
+ struct stat statbuf;
+ if (fstat(fd, &statbuf) < 0) {
+ std::string err = errno_to_str();
+ close(fd);
+ return Status::InternalError("failed to stat file {}: {}",
file.native(), err);
+ }
+ size_t file_len = statbuf.st_size;
+ CONSUME_THREAD_MEM_TRACKER(file_len);
+ void* buf = mmap(0, file_len, PROT_READ, MAP_SHARED, fd, 0);
Review Comment:
warning: use nullptr [modernize-use-nullptr]
```suggestion
void* buf = mmap(nullptr, file_len, PROT_READ, MAP_SHARED, fd, 0);
```
##########
be/src/tools/meta_tool.cpp:
##########
@@ -194,7 +188,7 @@
}
// 1. get dir
std::string dir;
- Status st = FileUtils::canonicalize(v[0], &dir);
+ Status st = io::global_local_filesystem()->canonicalize(v[0], &dir);
Review Comment:
warning: use of undeclared identifier 'io' [clang-diagnostic-error]
```cpp
Status st = io::global_local_filesystem()->canonicalize(v[0], &dir);
^
```
##########
be/src/tools/meta_tool.cpp:
##########
@@ -143,12 +142,7 @@ void delete_meta(DataDir* data_dir) {
Status init_data_dir(const std::string& dir, std::unique_ptr<DataDir>* ret) {
std::string root_path;
- Status st = FileUtils::canonicalize(dir, &root_path);
- if (!st.ok()) {
- std::cout << "invalid root path:" << FLAGS_root_path << ", error: " <<
st.to_string()
- << std::endl;
- return Status::InternalError("invalid root path");
- }
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(dir,
&root_path));
Review Comment:
warning: use of undeclared identifier 'io' [clang-diagnostic-error]
```cpp
RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(dir,
&root_path));
^
```
##########
be/test/olap/rowid_conversion_test.cpp:
##########
@@ -45,21 +45,17 @@ class TestRowIdConversion : public
testing::TestWithParam<std::tuple<KeysType, b
char buffer[MAX_PATH_LEN];
EXPECT_NE(getcwd(buffer, MAX_PATH_LEN), nullptr);
absolute_dir = std::string(buffer) + kTestDir;
-
- if (FileUtils::check_exist(absolute_dir)) {
- EXPECT_TRUE(FileUtils::remove_all(absolute_dir).ok());
- }
- EXPECT_TRUE(FileUtils::create_dir(absolute_dir).ok());
- EXPECT_TRUE(FileUtils::create_dir(absolute_dir + "/tablet_path").ok());
+
EXPECT_TRUE(io::global_local_filesystem()->delete_and_create_directory(absolute_dir).ok());
+ EXPECT_TRUE(io::global_local_filesystem()
+ ->create_directory(absolute_dir + "/tablet_path")
+ .ok());
doris::EngineOptions options;
k_engine = new StorageEngine(options);
StorageEngine::_s_instance = k_engine;
Review Comment:
warning: '_s_instance' is a private member of 'doris::StorageEngine'
[clang-diagnostic-error]
```cpp
StorageEngine::_s_instance = k_engine;
^
```
**be/src/olap/storage_engine.h:323:** declared private here
```cpp
_is_all_cluster_id_exist;
^
```
##########
be/test/io/fs/local_file_system_test.cpp:
##########
@@ -0,0 +1,263 @@
+// 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/fs/local_file_system.h"
+
+#include <algorithm>
+#include <filesystem>
+#include <fstream>
+#include <set>
+#include <vector>
+
+#include "common/status.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "io/fs/file_writer.h"
+
+namespace doris {
+
+class LocalFileSystemTest : public testing::Test {
+public:
+ // create a mock cgroup folder
+ virtual void SetUp() {
+ EXPECT_FALSE(std::filesystem::exists(_s_test_data_path));
+ bool exists = true;
+ EXPECT_TRUE(io::global_local_filesystem()->exists(_s_test_data_path,
&exists).ok());
+ EXPECT_FALSE(exists);
+ // create a mock cgroup path
+
EXPECT_TRUE(io::global_local_filesystem()->create_directory(_s_test_data_path).ok());
+ }
+
+ Status save_string_file(const std::filesystem::path& filename, const
std::string& content) {
+ io::FileWriterPtr file_writer;
+ RETURN_IF_ERROR(io::global_local_filesystem()->create_file(filename,
&file_writer));
+ RETURN_IF_ERROR(file_writer->append(content));
+ return file_writer->close();
+ }
+
+ bool check_exists(const std::string& file) {
+ bool exists = true;
+ EXPECT_TRUE(io::global_local_filesystem()->exists(file, &exists).ok());
+ return exists;
+ }
+
+ bool is_dir(const std::string& path) {
+ bool is_dir = true;
+ EXPECT_TRUE(io::global_local_filesystem()->is_directory(path,
&is_dir).ok());
+ return is_dir;
+ }
+
+ Status list_dirs_files(const std::string& path, std::vector<std::string>*
dirs,
+ std::vector<std::string>* files) {
+ bool only_file = true;
+ if (dirs != nullptr) {
+ only_file = false;
+ }
+ std::vector<io::FileInfo> file_infos;
+ bool exists = true;
+ RETURN_IF_ERROR(io::global_local_filesystem()->list(path, only_file,
&file_infos, &exists));
+ for (auto& file_info : file_infos) {
+ if (file_info.is_file && files != nullptr) {
+ files->push_back(file_info.file_name);
+ }
+ if (!file_info.is_file && dirs != nullptr) {
+ dirs->push_back(file_info.file_name);
+ }
+ }
+ return Status::OK();
+ }
+
+ Status delete_file_paths(const std::vector<std::string>& ps) {
+ for (auto& p : ps) {
+ bool exists = true;
+ RETURN_IF_ERROR(io::global_local_filesystem()->exists(p, &exists));
+ if (!exists) {
+ continue;
+ }
+ bool is_dir = true;
+ RETURN_IF_ERROR(io::global_local_filesystem()->is_directory(p,
&is_dir));
+ if (is_dir) {
+
RETURN_IF_ERROR(io::global_local_filesystem()->delete_directory(p));
+ } else {
+ RETURN_IF_ERROR(io::global_local_filesystem()->delete_file(p));
+ }
+ }
+ return Status::OK();
+ }
+
+ // delete the mock cgroup folder
+ virtual void TearDown() {
Review Comment:
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
[modernize-use-override]
```suggestion
void TearDown() override {
```
##########
be/test/vec/olap/vertical_compaction_test.cpp:
##########
@@ -50,19 +50,17 @@ class VerticalCompactionTest : public ::testing::Test {
EXPECT_NE(getcwd(buffer, MAX_PATH_LEN), nullptr);
absolute_dir = std::string(buffer) + kTestDir;
- if (FileUtils::check_exist(absolute_dir)) {
- EXPECT_TRUE(FileUtils::remove_all(absolute_dir).ok());
- }
- EXPECT_TRUE(FileUtils::create_dir(absolute_dir).ok());
- EXPECT_TRUE(FileUtils::create_dir(absolute_dir + "/tablet_path").ok());
+
EXPECT_TRUE(io::global_local_filesystem()->delete_and_create_directory(absolute_dir).ok());
+ EXPECT_TRUE(io::global_local_filesystem()
+ ->create_directory(absolute_dir + "/tablet_path")
+ .ok());
+
doris::EngineOptions options;
k_engine = new StorageEngine(options);
StorageEngine::_s_instance = k_engine;
Review Comment:
warning: '_s_instance' is a private member of 'doris::StorageEngine'
[clang-diagnostic-error]
```cpp
StorageEngine::_s_instance = k_engine;
^
```
**be/src/olap/storage_engine.h:323:** declared private here
```cpp
_is_all_cluster_id_exist;
^
```
##########
be/src/util/disk_info_mac.cpp:
##########
@@ -134,8 +134,11 @@ Status DiskInfo::get_disk_devices(const
std::vector<std::string>& paths,
std::vector<std::string> real_paths;
for (const auto& path : paths) {
std::string p;
- WARN_IF_ERROR(FileUtils::canonicalize(path, &p),
- "canonicalize path " + path + " failed, skip disk
monitoring of this path");
+ Status st = io::global_local_filesystem()->canonicalize(path, &p);
+ if (!st.ok()) {
+ LOG(WARN) << "skip disk monitoring of path. " << st;
Review Comment:
warning: use of undeclared identifier 'COMPACT_GOOGLE_LOG_WARN'
[clang-diagnostic-error]
```cpp
LOG(WARN) << "skip disk monitoring of path. " << st;
^
```
expanded from here
##########
be/test/io/fs/local_file_system_test.cpp:
##########
@@ -0,0 +1,263 @@
+// 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/fs/local_file_system.h"
+
+#include <algorithm>
+#include <filesystem>
+#include <fstream>
+#include <set>
+#include <vector>
+
+#include "common/status.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "io/fs/file_writer.h"
+
+namespace doris {
+
+class LocalFileSystemTest : public testing::Test {
+public:
+ // create a mock cgroup folder
+ virtual void SetUp() {
Review Comment:
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
[modernize-use-override]
```suggestion
void SetUp() override {
```
##########
be/src/tools/meta_tool.cpp:
##########
@@ -334,7 +328,7 @@
show_meta();
} else if (FLAGS_operation == "batch_delete_meta") {
std::string tablet_file;
- Status st = FileUtils::canonicalize(FLAGS_tablet_file, &tablet_file);
+ Status st =
io::global_local_filesystem()->canonicalize(FLAGS_tablet_file, &tablet_file);
Review Comment:
warning: use of undeclared identifier 'io' [clang-diagnostic-error]
```cpp
Status st =
io::global_local_filesystem()->canonicalize(FLAGS_tablet_file, &tablet_file);
^
```
##########
be/test/runtime/test_env.cc:
##########
@@ -36,16 +36,6 @@ TestEnv::TestEnv() {
// TODO may need rpc support, etc.
}
-void TestEnv::init_tmp_file_mgr(const std::vector<std::string>& tmp_dirs, bool
one_dir_per_device) {
- _tmp_file_mgr = std::make_shared<TmpFileMgr>();
- _exec_env->_tmp_file_mgr = _tmp_file_mgr.get();
-
- DiskInfo::init();
- // will use DiskInfo::num_disks(), DiskInfo should be initialized before
- auto st = _tmp_file_mgr->init_custom(tmp_dirs, one_dir_per_device);
- DCHECK(st.ok()) << st;
-}
-
TestEnv::~TestEnv() {
SAFE_DELETE(_exec_env->_result_queue_mgr);
Review Comment:
warning: '_result_queue_mgr' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
SAFE_DELETE(_exec_env->_result_queue_mgr);
^
```
**be/src/runtime/exec_env.h:198:** declared private here
```cpp
ResultQueueMgr* _result_queue_mgr = nullptr;
^
```
##########
be/test/olap/tablet_mgr_test.cpp:
##########
@@ -99,7 +98,8 @@ TEST_F(TabletMgrTest, CreateTablet) {
TabletSharedPtr tablet = _tablet_mgr->get_tablet(111);
Review Comment:
warning: '_tablet_mgr' is a private member of 'doris::TabletMgrTest'
[clang-diagnostic-error]
```cpp
TabletSharedPtr tablet = _tablet_mgr->get_tablet(111);
^
```
**be/test/olap/tablet_mgr_test.cpp:71:** declared private here
```cpp
TabletManager* _tablet_mgr = nullptr;
^
```
##########
be/test/olap/tablet_mgr_test.cpp:
##########
@@ -158,8 +158,9 @@
TabletSharedPtr tablet = _tablet_mgr->get_tablet(111);
EXPECT_TRUE(tablet != nullptr);
// check dir exist
- bool dir_exist = FileUtils::check_exist(tablet->tablet_path());
- EXPECT_TRUE(dir_exist) << tablet->tablet_path();
+ bool dir_exist = false;
+ EXPECT_TRUE(io::global_local_filesystem()->exists(tablet->tablet_path(),
&dir_exist).ok());
+ EXPECT_TRUE(dir_exist);
// check meta has this tablet
TabletMetaSharedPtr new_tablet_meta(new TabletMeta());
Status check_meta_st = TabletMetaManager::get_meta(_data_dir, 111, 3333,
new_tablet_meta);
Review Comment:
warning: '_data_dir' is a private member of 'doris::TabletMgrTest'
[clang-diagnostic-error]
```cpp
Status check_meta_st = TabletMetaManager::get_meta(_data_dir, 111, 3333,
new_tablet_meta);
^
```
**be/test/olap/tablet_mgr_test.cpp:69:** declared private here
```cpp
DataDir* _data_dir = nullptr;
^
```
##########
be/test/olap/tablet_mgr_test.cpp:
##########
@@ -158,8 +158,9 @@
TabletSharedPtr tablet = _tablet_mgr->get_tablet(111);
Review Comment:
warning: '_tablet_mgr' is a private member of 'doris::TabletMgrTest'
[clang-diagnostic-error]
```cpp
TabletSharedPtr tablet = _tablet_mgr->get_tablet(111);
^
```
**be/test/olap/tablet_mgr_test.cpp:71:** declared private here
```cpp
TabletManager* _tablet_mgr = nullptr;
^
```
--
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]
