You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "platoneko (via GitHub)" <gi...@apache.org> on 2023/10/25 15:18:35 UTC
[PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
platoneko opened a new pull request, #25921:
URL: https://github.com/apache/doris/pull/25921
## Proposed changes
Decouple rowset id generator and local rowsets garbage collection implementation.
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1374780496
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
Review Comment:
warning: 'gen_cpp/Types_types.h' file not found [clang-diagnostic-error]
```cpp
#include <gen_cpp/Types_types.h>
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
Review Comment:
warning: all parameters should be named in a function [readability-named-parameter]
```suggestion
TEST(PathGcTest /*unused*/, GcTabletAndRowset /*unused*/) {
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
+ std::vector<std::string_view> expected_paths;
+ for (auto&& tablet : active_tablets) {
+ expected_paths.emplace_back(tablet->tablet_path());
+ }
+ std::sort(expected_paths.begin(), expected_paths.end());
+ std::sort(paths.begin(), paths.end());
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(paths[i], expected_paths[i]);
+ }
+
+ // Prepare rowsets
+ auto rng = std::default_random_engine {static_cast<uint32_t>(::time(nullptr))};
+ std::uniform_int_distribution<int64_t> u(0, active_tablets.size() - 1);
+ auto create_rowset = [&]() {
+ auto rowset_meta = std::make_shared<RowsetMeta>();
+ auto&& tablet = active_tablets[u(rng)];
+ rowset_meta->set_tablet_id(tablet->tablet_id());
+ rowset_meta->set_tablet_uid(tablet->tablet_uid());
+ rowset_meta->set_rowset_id(engine.next_rowset_id());
+ return std::make_shared<BetaRowset>(tablet->tablet_schema(), tablet->tablet_path(),
+ std::move(rowset_meta));
+ };
+ // tablet_id -> filenames
+ std::unordered_map<int64_t, std::vector<std::string>> expected_rowset_files;
+ auto create_rowset_files = [&](const BetaRowset& rs, bool is_garbage) {
+ auto& filenames = expected_rowset_files[rs.rowset_meta()->tablet_id()];
+ std::unique_ptr<io::FileWriter> writer;
+ auto filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 0);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 0, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 1);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 1, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ return writer->close();
+ };
+ // Prepare pending rowsets
+ for (int i = 0; i < 20; ++i) {
Review Comment:
warning: 20 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int i = 0; i < 20; ++i) {
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
+ std::vector<std::string_view> expected_paths;
+ for (auto&& tablet : active_tablets) {
+ expected_paths.emplace_back(tablet->tablet_path());
+ }
+ std::sort(expected_paths.begin(), expected_paths.end());
+ std::sort(paths.begin(), paths.end());
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(paths[i], expected_paths[i]);
+ }
+
+ // Prepare rowsets
+ auto rng = std::default_random_engine {static_cast<uint32_t>(::time(nullptr))};
+ std::uniform_int_distribution<int64_t> u(0, active_tablets.size() - 1);
+ auto create_rowset = [&]() {
+ auto rowset_meta = std::make_shared<RowsetMeta>();
+ auto&& tablet = active_tablets[u(rng)];
+ rowset_meta->set_tablet_id(tablet->tablet_id());
+ rowset_meta->set_tablet_uid(tablet->tablet_uid());
+ rowset_meta->set_rowset_id(engine.next_rowset_id());
+ return std::make_shared<BetaRowset>(tablet->tablet_schema(), tablet->tablet_path(),
+ std::move(rowset_meta));
+ };
+ // tablet_id -> filenames
+ std::unordered_map<int64_t, std::vector<std::string>> expected_rowset_files;
+ auto create_rowset_files = [&](const BetaRowset& rs, bool is_garbage) {
+ auto& filenames = expected_rowset_files[rs.rowset_meta()->tablet_id()];
+ std::unique_ptr<io::FileWriter> writer;
+ auto filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 0);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 0, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 1);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 1, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ return writer->close();
+ };
+ // Prepare pending rowsets
+ for (int i = 0; i < 20; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.pending_local_rowsets().add(rs->rowset_id());
+ }
+ // Prepare unused rowsets
+ for (int i = 0; i < 30; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.add_unused_rowset(std::move(rs));
+ }
+ // Prepare visible rowsets
+ for (int i = 0; i < 30; ++i) {
Review Comment:
warning: 30 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int i = 0; i < 30; ++i) {
^
```
##########
be/test/olap/rowset/pending_rowset_test.cpp:
##########
@@ -0,0 +1,68 @@
+// 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 <gtest/gtest.h>
+
+#include "olap/rowset/beta_rowset_writer.h"
+#include "olap/rowset/pending_rowset_helper.h"
+#include "olap/rowset/pending_rowset_mixin.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_schema.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PendingRowsetTest, PendingRowsetWriter) {
+ Defer defer {[] { ExecEnv::GetInstance()->set_storage_engine(nullptr); }};
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+
+ auto rowset_id = engine.next_rowset_id();
+ std::unique_ptr<BetaRowsetWriter> writer =
+ std::make_unique<PendingRowsetMixin<BetaRowsetWriter>>();
+ RowsetWriterContext ctx;
+ ctx.rowset_id = rowset_id;
+ ctx.tablet_schema = std::make_shared<TabletSchema>();
+ auto st = writer->init(ctx);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_TRUE(engine.pending_local_rowsets().contains(rowset_id));
+ writer.reset();
+ EXPECT_FALSE(engine.pending_local_rowsets().contains(rowset_id));
+}
+
+TEST(PendingRowsetTest, PendingRowsetGuard) {
Review Comment:
warning: all parameters should be named in a function [readability-named-parameter]
```suggestion
TEST(PendingRowsetTest /*unused*/, PendingRowsetGuard /*unused*/) {
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
Review Comment:
warning: 20 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
ASSERT_EQ(paths.size(), 20);
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
Review Comment:
warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int64_t i = 0; i < 10; ++i) {
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
Review Comment:
warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
ASSERT_EQ(paths.size(), 10);
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
Review Comment:
warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int64_t i = 0; i < 10; ++i) {
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
Review Comment:
warning: 10000 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
int64_t next_tablet_id = 10000;
^
```
##########
be/test/olap/rowset/pending_rowset_test.cpp:
##########
@@ -0,0 +1,68 @@
+// 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 <gtest/gtest.h>
Review Comment:
warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]
```cpp
#include <gtest/gtest.h>
^
```
##########
be/test/olap/rowset/pending_rowset_test.cpp:
##########
@@ -0,0 +1,68 @@
+// 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 <gtest/gtest.h>
+
+#include "olap/rowset/beta_rowset_writer.h"
+#include "olap/rowset/pending_rowset_helper.h"
+#include "olap/rowset/pending_rowset_mixin.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_schema.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PendingRowsetTest, PendingRowsetWriter) {
Review Comment:
warning: all parameters should be named in a function [readability-named-parameter]
```suggestion
TEST(PendingRowsetTest /*unused*/, PendingRowsetWriter /*unused*/) {
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
+ std::vector<std::string_view> expected_paths;
+ for (auto&& tablet : active_tablets) {
+ expected_paths.emplace_back(tablet->tablet_path());
+ }
+ std::sort(expected_paths.begin(), expected_paths.end());
+ std::sort(paths.begin(), paths.end());
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(paths[i], expected_paths[i]);
+ }
+
+ // Prepare rowsets
+ auto rng = std::default_random_engine {static_cast<uint32_t>(::time(nullptr))};
+ std::uniform_int_distribution<int64_t> u(0, active_tablets.size() - 1);
+ auto create_rowset = [&]() {
+ auto rowset_meta = std::make_shared<RowsetMeta>();
+ auto&& tablet = active_tablets[u(rng)];
+ rowset_meta->set_tablet_id(tablet->tablet_id());
+ rowset_meta->set_tablet_uid(tablet->tablet_uid());
+ rowset_meta->set_rowset_id(engine.next_rowset_id());
+ return std::make_shared<BetaRowset>(tablet->tablet_schema(), tablet->tablet_path(),
+ std::move(rowset_meta));
+ };
+ // tablet_id -> filenames
+ std::unordered_map<int64_t, std::vector<std::string>> expected_rowset_files;
+ auto create_rowset_files = [&](const BetaRowset& rs, bool is_garbage) {
+ auto& filenames = expected_rowset_files[rs.rowset_meta()->tablet_id()];
+ std::unique_ptr<io::FileWriter> writer;
+ auto filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 0);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 0, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 1);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 1, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ return writer->close();
+ };
+ // Prepare pending rowsets
+ for (int i = 0; i < 20; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.pending_local_rowsets().add(rs->rowset_id());
+ }
+ // Prepare unused rowsets
+ for (int i = 0; i < 30; ++i) {
Review Comment:
warning: 30 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int i = 0; i < 30; ++i) {
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
+ std::vector<std::string_view> expected_paths;
+ for (auto&& tablet : active_tablets) {
+ expected_paths.emplace_back(tablet->tablet_path());
+ }
+ std::sort(expected_paths.begin(), expected_paths.end());
+ std::sort(paths.begin(), paths.end());
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(paths[i], expected_paths[i]);
+ }
+
+ // Prepare rowsets
+ auto rng = std::default_random_engine {static_cast<uint32_t>(::time(nullptr))};
+ std::uniform_int_distribution<int64_t> u(0, active_tablets.size() - 1);
+ auto create_rowset = [&]() {
+ auto rowset_meta = std::make_shared<RowsetMeta>();
+ auto&& tablet = active_tablets[u(rng)];
+ rowset_meta->set_tablet_id(tablet->tablet_id());
+ rowset_meta->set_tablet_uid(tablet->tablet_uid());
+ rowset_meta->set_rowset_id(engine.next_rowset_id());
+ return std::make_shared<BetaRowset>(tablet->tablet_schema(), tablet->tablet_path(),
+ std::move(rowset_meta));
+ };
+ // tablet_id -> filenames
+ std::unordered_map<int64_t, std::vector<std::string>> expected_rowset_files;
+ auto create_rowset_files = [&](const BetaRowset& rs, bool is_garbage) {
+ auto& filenames = expected_rowset_files[rs.rowset_meta()->tablet_id()];
+ std::unique_ptr<io::FileWriter> writer;
+ auto filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 0);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 0, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 1);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 1, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ return writer->close();
+ };
+ // Prepare pending rowsets
+ for (int i = 0; i < 20; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.pending_local_rowsets().add(rs->rowset_id());
+ }
+ // Prepare unused rowsets
+ for (int i = 0; i < 30; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.add_unused_rowset(std::move(rs));
+ }
+ // Prepare visible rowsets
+ for (int i = 0; i < 30; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ auto tablet = engine.tablet_manager()->get_tablet(rs->rowset_meta()->tablet_id());
+ ASSERT_TRUE(tablet) << rs->rowset_meta()->tablet_id();
+ auto max_version = tablet->max_version_unlocked().second;
+ rs->rowset_meta()->set_version({max_version + 1, max_version + 1});
+ st = tablet->add_inc_rowset(rs);
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare rowsets in OlapMeta
+ for (int i = 0; i < 20; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ st = RowsetMetaManager::save(data_dir.get_meta(), rs->rowset_meta()->tablet_uid(),
+ rs->rowset_id(), rs->rowset_meta()->get_rowset_pb());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage rowset files
+ for (int i = 0; i < 20; ++i) {
Review Comment:
warning: 20 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int i = 0; i < 20; ++i) {
^
```
##########
be/test/olap/path_gc_test.cpp:
##########
@@ -0,0 +1,205 @@
+// 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 <gen_cpp/Types_types.h>
+#include <gtest/gtest.h>
+
+#include <random>
+
+#include "io/fs/file_writer.h"
+#include "io/fs/local_file_system.h"
+#include "olap/olap_common.h"
+#include "olap/rowset/beta_rowset.h"
+#include "olap/rowset/rowset_meta_manager.h"
+#include "olap/storage_engine.h"
+#include "olap/tablet_fwd.h"
+#include "olap/tablet_manager.h"
+#include "runtime/exec_env.h"
+
+namespace doris {
+
+TEST(PathGcTest, GcTabletAndRowset) {
+ const std::string dir_path = "ut_dir/path_gc_test";
+ Defer defer {[&] {
+ ExecEnv::GetInstance()->set_storage_engine(nullptr);
+ auto st = io::global_local_filesystem()->delete_directory(dir_path);
+ }};
+ auto&& fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+ st = fs->create_directory(dir_path);
+ ASSERT_TRUE(st.ok()) << st;
+
+ StorageEngine engine({});
+ ExecEnv::GetInstance()->set_storage_engine(&engine);
+ DataDir data_dir(dir_path, -1, TStorageMedium::HDD, engine.tablet_manager());
+ st = data_dir._init_meta();
+ ASSERT_TRUE(st.ok()) << st;
+
+ // Prepare tablets
+ auto create_tablet = [&](int64_t tablet_id) {
+ auto tablet_meta = std::make_shared<TabletMeta>();
+ tablet_meta->_tablet_id = tablet_id;
+ tablet_meta->set_tablet_uid({tablet_id, 0});
+ tablet_meta->set_shard_id(tablet_id % 4);
+ tablet_meta->_schema_hash = tablet_id;
+ auto tablet = std::make_shared<Tablet>(std::move(tablet_meta), &data_dir);
+ auto& tablet_map = engine.tablet_manager()->_get_tablet_map(tablet_id);
+ tablet_map[tablet_id] = tablet;
+ return tablet;
+ };
+ std::vector<TabletSharedPtr> active_tablets;
+ int64_t next_tablet_id = 10000;
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ active_tablets.push_back(create_tablet(tablet_id));
+ }
+ // Prepare tablet directories
+ for (auto&& tablet : active_tablets) {
+ st = fs->create_directory(tablet->tablet_path());
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare garbage tablet directories
+ for (int64_t i = 0; i < 10; ++i) {
+ int64_t tablet_id = ++next_tablet_id;
+ // {dir_path}/data/{shard_id}/{tablet_id}/{schema_hash}
+ st = fs->create_directory(
+ fmt::format("{}/data/{}/{}/{}", dir_path, tablet_id % 4, tablet_id, tablet_id));
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ // Test path scan
+ auto paths = data_dir._perform_path_scan();
+ ASSERT_EQ(paths.size(), 20);
+
+ // Test tablet gc
+ config::path_gc_check_step = 0;
+ data_dir._perform_path_gc_by_tablet(paths);
+ ASSERT_EQ(paths.size(), 10);
+ std::vector<std::string_view> expected_paths;
+ for (auto&& tablet : active_tablets) {
+ expected_paths.emplace_back(tablet->tablet_path());
+ }
+ std::sort(expected_paths.begin(), expected_paths.end());
+ std::sort(paths.begin(), paths.end());
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(paths[i], expected_paths[i]);
+ }
+
+ // Prepare rowsets
+ auto rng = std::default_random_engine {static_cast<uint32_t>(::time(nullptr))};
+ std::uniform_int_distribution<int64_t> u(0, active_tablets.size() - 1);
+ auto create_rowset = [&]() {
+ auto rowset_meta = std::make_shared<RowsetMeta>();
+ auto&& tablet = active_tablets[u(rng)];
+ rowset_meta->set_tablet_id(tablet->tablet_id());
+ rowset_meta->set_tablet_uid(tablet->tablet_uid());
+ rowset_meta->set_rowset_id(engine.next_rowset_id());
+ return std::make_shared<BetaRowset>(tablet->tablet_schema(), tablet->tablet_path(),
+ std::move(rowset_meta));
+ };
+ // tablet_id -> filenames
+ std::unordered_map<int64_t, std::vector<std::string>> expected_rowset_files;
+ auto create_rowset_files = [&](const BetaRowset& rs, bool is_garbage) {
+ auto& filenames = expected_rowset_files[rs.rowset_meta()->tablet_id()];
+ std::unique_ptr<io::FileWriter> writer;
+ auto filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 0);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 0, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}.dat", rs.rowset_id().to_string(), 1);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ RETURN_IF_ERROR(writer->close());
+ filename = fmt::format("{}_{}_{}.idx", rs.rowset_id().to_string(), 1, 987);
+ RETURN_IF_ERROR(fs->create_file(rs._rowset_dir + '/' + filename, &writer));
+ if (!is_garbage) {
+ filenames.push_back(std::move(filename));
+ }
+ return writer->close();
+ };
+ // Prepare pending rowsets
+ for (int i = 0; i < 20; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.pending_local_rowsets().add(rs->rowset_id());
+ }
+ // Prepare unused rowsets
+ for (int i = 0; i < 30; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ engine.add_unused_rowset(std::move(rs));
+ }
+ // Prepare visible rowsets
+ for (int i = 0; i < 30; ++i) {
+ auto rs = create_rowset();
+ st = create_rowset_files(*rs, false);
+ ASSERT_TRUE(st.ok()) << st;
+ auto tablet = engine.tablet_manager()->get_tablet(rs->rowset_meta()->tablet_id());
+ ASSERT_TRUE(tablet) << rs->rowset_meta()->tablet_id();
+ auto max_version = tablet->max_version_unlocked().second;
+ rs->rowset_meta()->set_version({max_version + 1, max_version + 1});
+ st = tablet->add_inc_rowset(rs);
+ ASSERT_TRUE(st.ok()) << st;
+ }
+ // Prepare rowsets in OlapMeta
+ for (int i = 0; i < 20; ++i) {
Review Comment:
warning: 20 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
for (int i = 0; i < 20; ++i) {
^
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1371962965
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
+ if (tablet_paths.empty()) {
return;
}
- LOG(INFO) << "start to path gc by tablet schemahash.";
+ LOG(INFO) << "start to path gc by tablet, dir=" << _path;
+ // Use double pointer to avoid move elements after erase a element
+ auto forward = tablet_paths.begin();
+ auto backward = tablet_paths.end();
int counter = 0;
- for (const auto& path : _all_tablet_schemahash_paths) {
- ++counter;
+ do {
if (config::path_gc_check_step > 0 && counter % config::path_gc_check_step == 0) {
std::this_thread::sleep_for(
std::chrono::milliseconds(config::path_gc_check_step_interval_ms));
}
+ auto& path = *forward;
TTabletId tablet_id = -1;
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- // should not happen, because already check it is a valid tablet schema hash path in previous step
- // so that log fatal here
- if (tablet_id < 1 || schema_hash < 1) {
- LOG(WARNING) << "invalid tablet id " << tablet_id << " or schema hash " << schema_hash
- << ", path=" << path;
- continue;
- }
- TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id);
- if (tablet != nullptr) {
- // could find the tablet, then skip check it
- continue;
- }
- std::string data_dir_path =
- io::Path(path).parent_path().parent_path().parent_path().parent_path();
- DataDir* data_dir = StorageEngine::instance()->get_store(data_dir_path);
- if (data_dir == nullptr) {
- LOG(WARNING) << "could not find data dir for tablet path " << path;
+ auto tablet = _tablet_manager->get_tablet(tablet_id);
+ if (!tablet) {
+ _tablet_manager->try_delete_unused_tablet_path(this, tablet_id, schema_hash, path);
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- _tablet_manager->try_delete_unused_tablet_path(data_dir, tablet_id, schema_hash, path);
- }
- _all_tablet_schemahash_paths.clear();
- LOG(INFO) << "finished one time path gc by tablet.";
+ // could find the tablet, then skip check it
+ ++forward;
+ } while (forward != backward && !_stop_bg_worker);
+ tablet_paths.erase(backward, tablet_paths.end());
+ LOG(INFO) << "finish path gc by tablet, dir=" << _path;
}
-void DataDir::_perform_path_gc_by_rowsetid() {
- // init the set of valid path
- // validate the path in data dir
- if (_all_check_paths.empty()) {
+void DataDir::_perform_path_gc_by_rowset(const std::vector<std::string>& tablet_paths) {
Review Comment:
warning: method '_perform_path_gc_by_rowset' can be made static [readability-convert-member-functions-to-static]
```suggestion
static void DataDir::_perform_path_gc_by_rowset(const std::vector<std::string>& tablet_paths) {
```
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
+ if (tablet_paths.empty()) {
return;
}
- LOG(INFO) << "start to path gc by tablet schemahash.";
+ LOG(INFO) << "start to path gc by tablet, dir=" << _path;
+ // Use double pointer to avoid move elements after erase a element
+ auto forward = tablet_paths.begin();
+ auto backward = tablet_paths.end();
int counter = 0;
- for (const auto& path : _all_tablet_schemahash_paths) {
- ++counter;
+ do {
if (config::path_gc_check_step > 0 && counter % config::path_gc_check_step == 0) {
std::this_thread::sleep_for(
std::chrono::milliseconds(config::path_gc_check_step_interval_ms));
}
+ auto& path = *forward;
TTabletId tablet_id = -1;
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- // should not happen, because already check it is a valid tablet schema hash path in previous step
- // so that log fatal here
- if (tablet_id < 1 || schema_hash < 1) {
- LOG(WARNING) << "invalid tablet id " << tablet_id << " or schema hash " << schema_hash
- << ", path=" << path;
- continue;
- }
- TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id);
- if (tablet != nullptr) {
- // could find the tablet, then skip check it
- continue;
- }
- std::string data_dir_path =
- io::Path(path).parent_path().parent_path().parent_path().parent_path();
- DataDir* data_dir = StorageEngine::instance()->get_store(data_dir_path);
- if (data_dir == nullptr) {
- LOG(WARNING) << "could not find data dir for tablet path " << path;
+ auto tablet = _tablet_manager->get_tablet(tablet_id);
+ if (!tablet) {
+ _tablet_manager->try_delete_unused_tablet_path(this, tablet_id, schema_hash, path);
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- _tablet_manager->try_delete_unused_tablet_path(data_dir, tablet_id, schema_hash, path);
- }
- _all_tablet_schemahash_paths.clear();
- LOG(INFO) << "finished one time path gc by tablet.";
+ // could find the tablet, then skip check it
+ ++forward;
+ } while (forward != backward && !_stop_bg_worker);
+ tablet_paths.erase(backward, tablet_paths.end());
+ LOG(INFO) << "finish path gc by tablet, dir=" << _path;
}
-void DataDir::_perform_path_gc_by_rowsetid() {
- // init the set of valid path
- // validate the path in data dir
- if (_all_check_paths.empty()) {
+void DataDir::_perform_path_gc_by_rowset(const std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (_stop_bg_worker) { return;
}
```
##########
be/src/olap/data_dir.cpp:
##########
@@ -704,55 +690,112 @@
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
continue;
}
- if (tablet_id > 0 && schema_hash > 0) {
- // tablet schema hash path or rowset file path
- // gc thread should get tablet include deleted tablet
- // or it will delete rowset file before tablet is garbage collected
- RowsetId rowset_id;
- bool is_rowset_file = TabletManager::get_rowset_id_from_path(path, &rowset_id);
- if (is_rowset_file) {
- TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id);
- if (tablet != nullptr) {
- if (!tablet->check_rowset_id(rowset_id) &&
- !StorageEngine::instance()->check_rowset_id_in_unused_rowsets(rowset_id)) {
- _process_garbage_path(path);
- }
+ bool exists;
+ std::vector<io::FileInfo> files;
+ if (_stop_bg_worker) break;
+ auto st = io::global_local_filesystem()->list(path, true, &files, &exists);
+ if (!st.ok()) [[unlikely]] {
+ LOG(WARNING) << "fail to list tablet path " << path << " : " << st;
+ continue;
+ }
+ std::unordered_set<std::string> useful_rowsets;
+ auto tablet = _tablet_manager->get_tablet(tablet_id);
+ if (!tablet) {
+ // Could not found the tablet, maybe it's a dropped tablet, will be reclaimed
+ // in the next time `_perform_path_gc_by_tablet`
+ continue;
+ }
+ tablet->traverse_rowsets(
+ [&useful_rowsets](auto& rs) { useful_rowsets.insert(rs->rowset_id().to_string()); },
+ true);
+ std::unordered_map<std::string, bool> checked_rowsets; // rowset_id -> is_garbage
+ // FIXME(plat1ko): Abstract a common function in `olap_common.h`
+ auto get_rowset_id = [](std::string_view filename) -> std::string {
+ if (filename.ends_with(".dat")) {
+ // filename format: {rowset_id}_{segment_num}.dat
+ auto end = filename.rfind('_');
+ if (end == std::string::npos) {
+ return "";
+ }
+ return std::string(filename.substr(0, end));
+ }
+ if (filename.ends_with(".idx")) {
+ // filename format: {rowset_id}_{segment_num}_{index_id}.idx
+ auto end = filename.find('_');
+ if (end == std::string::npos) {
+ return "";
}
+ return std::string(filename.substr(0, end));
+ }
+ return "";
+ };
+ auto reclaim_rowset_file = [](const std::string& path) {
+ auto st = io::global_local_filesystem()->delete_file(path);
+ if (!st.ok()) [[unlikely]] {
+ LOG(WARNING) << "failed to delete garbage rowset file: " << st;
+ return;
+ }
+ LOG(INFO) << "delete garbage path: " << path; // Audit log
+ };
+ auto should_reclaim = [&, this](const std::string& rowset_id) {
+ return !useful_rowsets.contains(rowset_id) &&
+ !StorageEngine::instance()->pending_local_rowsets().contains(rowset_id) &&
+ !StorageEngine::instance()->check_rowset_id_in_unused_rowsets(rowset_id) &&
+ RowsetMetaManager::exists(get_meta(), tablet->tablet_uid(), rowset_id)
+ .is<META_KEY_NOT_FOUND>();
+ };
+ for (auto&& file : files) {
+ auto rowset_id = get_rowset_id(file.file_name);
+ if (rowset_id.empty()) {
+ continue; // Not a rowset
+ }
+ auto find_it = checked_rowsets.find(rowset_id);
+ if (find_it != checked_rowsets.end()) {
+ if (find_it->second) {
+ reclaim_rowset_file(path + '/' + file.file_name);
+ }
+ continue;
+ }
+ // Check rowset useful
+ if (should_reclaim(rowset_id)) {
+ reclaim_rowset_file(path + '/' + file.file_name);
+ checked_rowsets.emplace(std::move(rowset_id), true);
+ } else {
+ checked_rowsets.emplace(std::move(rowset_id), false);
}
}
}
- _all_check_paths.clear();
- LOG(INFO) << "finished one time path gc by rowsetid.";
+ LOG(INFO) << "finish path gc by rowset.";
}
// path producer
-Status DataDir::perform_path_scan() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- if (!_all_check_paths.empty()) {
- LOG(INFO) << "_all_check_paths is not empty when path scan.";
- return Status::OK();
- }
- LOG(INFO) << "start to scan data dir path:" << _path;
+std::vector<std::string> DataDir::_perform_path_scan() {
+ std::vector<std::string> tablet_paths;
+ if (_stop_bg_worker) return tablet_paths;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (_stop_bg_worker) { return tablet_paths;
}
```
##########
be/src/olap/single_replica_compaction.h:
##########
@@ -62,6 +63,9 @@ class SingleReplicaCompaction : public Compaction {
CompactionType _compaction_type;
DISALLOW_COPY_AND_ASSIGN(SingleReplicaCompaction);
+
+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/olap/single_replica_compaction.h:47:** previously declared here
```cpp
private:
^
```
</details>
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@ Status DataDir::load() {
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (_stop_bg_worker) { return;
}
```
##########
be/src/olap/rowset/rowset_meta_manager.cpp:
##########
@@ -47,7 +47,12 @@ bool RowsetMetaManager::check_rowset_meta(OlapMeta* meta, TabletUid tablet_uid,
}
Status RowsetMetaManager::exists(OlapMeta* meta, TabletUid tablet_uid, const RowsetId& rowset_id) {
- std::string key = ROWSET_PREFIX + tablet_uid.to_string() + "_" + rowset_id.to_string();
+ return exists(meta, tablet_uid, rowset_id.to_string());
+}
+
+Status RowsetMetaManager::exists(OlapMeta* meta, TabletUid tablet_uid,
Review Comment:
warning: method 'exists' can be made static [readability-convert-member-functions-to-static]
```suggestion
static Status RowsetMetaManager::exists(OlapMeta* meta, TabletUid tablet_uid,
```
##########
be/src/olap/storage_engine.cpp:
##########
@@ -1245,9 +1244,9 @@ Status StorageEngine::execute_task(EngineTask* task) {
}
// check whether any unused rowsets's id equal to rowset_id
-bool StorageEngine::check_rowset_id_in_unused_rowsets(const RowsetId& rowset_id) {
+bool StorageEngine::check_rowset_id_in_unused_rowsets(const std::string& rowset_id) {
Review Comment:
warning: method 'check_rowset_id_in_unused_rowsets' can be made static [readability-convert-member-functions-to-static]
```suggestion
s id equal to rowset_idstatic
```
##########
be/src/olap/tablet.h:
##########
@@ -516,11 +514,16 @@ class Tablet final : public BaseTablet {
RowsetSharedPtr get_rowset(const RowsetId& rowset_id);
- void traverse_rowsets(std::function<void(const RowsetSharedPtr&)> visitor) {
+ void traverse_rowsets(std::function<void(const RowsetSharedPtr&)> visitor,
Review Comment:
warning: method 'traverse_rowsets' can be made static [readability-convert-member-functions-to-static]
```suggestion
static void traverse_rowsets(std::function<void(const RowsetSharedPtr&)> visitor,
```
##########
be/src/olap/txn_manager.cpp:
##########
@@ -62,6 +62,36 @@ using std::vector;
namespace doris {
using namespace ErrorCode;
+struct TabletTxnInfo {
+ PUniqueId load_id;
+ RowsetSharedPtr rowset;
+ PendingRowsetGuard pending_rs_guard;
+ bool unique_key_merge_on_write {false};
+ DeleteBitmapPtr delete_bitmap;
+ // records rowsets calc in commit txn
+ RowsetIdUnorderedSet rowset_ids;
+ int64_t creation_time;
+ bool ingest {false};
+ std::shared_ptr<PartialUpdateInfo> partial_update_info;
+
+ TabletTxnInfo(PUniqueId load_id, RowsetSharedPtr rowset)
+ : load_id(load_id), rowset(rowset), creation_time(UnixSeconds()) {}
+
+ TabletTxnInfo(PUniqueId load_id, RowsetSharedPtr rowset, bool ingest_arg)
+ : load_id(load_id), rowset(rowset), creation_time(UnixSeconds()), ingest(ingest_arg) {}
+
+ TabletTxnInfo(PUniqueId load_id, RowsetSharedPtr rowset, bool merge_on_write,
+ DeleteBitmapPtr delete_bitmap, const RowsetIdUnorderedSet& ids)
+ : load_id(load_id),
+ rowset(rowset),
+ unique_key_merge_on_write(merge_on_write),
+ delete_bitmap(delete_bitmap),
+ rowset_ids(ids),
+ creation_time(UnixSeconds()) {}
+
+ TabletTxnInfo() {}
Review Comment:
warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]
```suggestion
TabletTxnInfo() = default;
```
##########
be/src/olap/tablet.h:
##########
@@ -516,11 +514,16 @@
RowsetSharedPtr get_rowset(const RowsetId& rowset_id);
- void traverse_rowsets(std::function<void(const RowsetSharedPtr&)> visitor) {
+ void traverse_rowsets(std::function<void(const RowsetSharedPtr&)> visitor,
+ bool include_stale = false) {
std::shared_lock rlock(_meta_lock);
for (auto& [v, rs] : _rs_version_map) {
visitor(rs);
}
+ if (!include_stale) return;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (!include_stale) { return;
}
```
##########
be/test/olap/txn_manager_test.cpp:
##########
@@ -100,6 +100,14 @@ class TxnManagerTest : public testing::Test {
_schema->init_from_pb(tablet_schema_pb);
}
+ void create_tablet() {
Review Comment:
warning: method 'create_tablet' can be made static [readability-convert-member-functions-to-static]
```suggestion
static void create_tablet() {
```
##########
be/test/olap/txn_manager_test.cpp:
##########
@@ -194,137 +202,153 @@
// 2. commit txn
// 3. should be success
TEST_F(TxnManagerTest, CommitTxnWithPrepare) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- static_cast<void>(_txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false));
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. commit without prepare
// 2. should success
TEST_F(TxnManagerTest, CommitTxnWithNoPrepare) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. commit twice with different rowset id
// 2. should failed
TEST_F(TxnManagerTest, CommitTxnTwiceWithDiffRowsetId) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- load_id, _rowset_diff_id, false);
- EXPECT_TRUE(status != Status::OK());
+ auto rowset_id1 = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard1 {rowset_id1};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard1), false);
+ ASSERT_TRUE(st.ok()) << st;
+ auto rowset_id2 = _rowset_diff_id->rowset_id().to_string();
+ PendingRowsetGuard guard2 {rowset_id2};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset_diff_id, std::move(guard2), false);
+ ASSERT_FALSE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id1));
+ EXPECT_FALSE(k_engine->pending_local_rowsets().contains(rowset_id2));
}
// 1. commit twice with same rowset id
// 2. should success
TEST_F(TxnManagerTest, CommitTxnTwiceWithSameRowsetId) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- load_id, _rowset_same_id, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard1 {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard1), false);
+ ASSERT_TRUE(st.ok()) << st;
+ PendingRowsetGuard guard2 {rowset_id};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset_same_id, std::move(guard2), false);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. prepare twice should be success
TEST_F(TxnManagerTest, PrepareNewTxnTwice) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
}
// 1. txn could be rollbacked if it is not committed
TEST_F(TxnManagerTest, RollbackNotCommittedTxn) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status != Status::OK());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_FALSE(st.ok()) << st;
}
// 1. txn could not be rollbacked if it is committed
TEST_F(TxnManagerTest, RollbackCommittedTxn) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
- EXPECT_FALSE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
+ ASSERT_FALSE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. publish version success
TEST_F(TxnManagerTest, PublishVersionSuccessful) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
Version new_version(10, 11);
TabletPublishStatistics stats;
- status = _txn_mgr->publish_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- new_version, &stats);
- EXPECT_TRUE(status == Status::OK());
+ st = _txn_mgr->publish_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ new_version, &stats);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
- // FIXME(Drogon): these is wrong when not real tablet exist
- // EXPECT_EQ(rowset_meta->start_version(), 10);
- // EXPECT_EQ(rowset_meta->end_version(), 11);
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_FALSE(k_engine->pending_local_rowsets().contains(rowset_id));
+ EXPECT_EQ(rowset_meta->start_version(), 10);
+ EXPECT_EQ(rowset_meta->end_version(), 11);
Review Comment:
warning: 11 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
EXPECT_EQ(rowset_meta->end_version(), 11);
^
```
##########
be/test/olap/txn_manager_test.cpp:
##########
@@ -194,137 +202,153 @@
// 2. commit txn
// 3. should be success
TEST_F(TxnManagerTest, CommitTxnWithPrepare) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- static_cast<void>(_txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false));
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. commit without prepare
// 2. should success
TEST_F(TxnManagerTest, CommitTxnWithNoPrepare) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. commit twice with different rowset id
// 2. should failed
TEST_F(TxnManagerTest, CommitTxnTwiceWithDiffRowsetId) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- load_id, _rowset_diff_id, false);
- EXPECT_TRUE(status != Status::OK());
+ auto rowset_id1 = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard1 {rowset_id1};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard1), false);
+ ASSERT_TRUE(st.ok()) << st;
+ auto rowset_id2 = _rowset_diff_id->rowset_id().to_string();
+ PendingRowsetGuard guard2 {rowset_id2};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset_diff_id, std::move(guard2), false);
+ ASSERT_FALSE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id1));
+ EXPECT_FALSE(k_engine->pending_local_rowsets().contains(rowset_id2));
}
// 1. commit twice with same rowset id
// 2. should success
TEST_F(TxnManagerTest, CommitTxnTwiceWithSameRowsetId) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- load_id, _rowset_same_id, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard1 {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard1), false);
+ ASSERT_TRUE(st.ok()) << st;
+ PendingRowsetGuard guard2 {rowset_id};
+ st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid, load_id,
+ _rowset_same_id, std::move(guard2), false);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. prepare twice should be success
TEST_F(TxnManagerTest, PrepareNewTxnTwice) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
}
// 1. txn could be rollbacked if it is not committed
TEST_F(TxnManagerTest, RollbackNotCommittedTxn) {
- Status status =
- _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
- EXPECT_TRUE(status == Status::OK());
+ auto st = _txn_mgr->prepare_txn(partition_id, transaction_id, tablet_id, _tablet_uid, load_id);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status != Status::OK());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_FALSE(st.ok()) << st;
}
// 1. txn could not be rollbacked if it is committed
TEST_F(TxnManagerTest, RollbackCommittedTxn) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
- status = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
- EXPECT_FALSE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
+ st = _txn_mgr->rollback_txn(partition_id, transaction_id, tablet_id, _tablet_uid);
+ ASSERT_FALSE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_TRUE(k_engine->pending_local_rowsets().contains(rowset_id));
}
// 1. publish version success
TEST_F(TxnManagerTest, PublishVersionSuccessful) {
- Status status = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id,
- _tablet_uid, load_id, _rowset, false);
- EXPECT_TRUE(status == Status::OK());
+ auto rowset_id = _rowset->rowset_id().to_string();
+ PendingRowsetGuard guard {rowset_id};
+ auto st = _txn_mgr->commit_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ load_id, _rowset, std::move(guard), false);
+ ASSERT_TRUE(st.ok()) << st;
Version new_version(10, 11);
TabletPublishStatistics stats;
- status = _txn_mgr->publish_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
- new_version, &stats);
- EXPECT_TRUE(status == Status::OK());
+ st = _txn_mgr->publish_txn(_meta, partition_id, transaction_id, tablet_id, _tablet_uid,
+ new_version, &stats);
+ ASSERT_TRUE(st.ok()) << st;
RowsetMetaSharedPtr rowset_meta(new RowsetMeta());
- status = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(),
- rowset_meta);
- EXPECT_TRUE(status == Status::OK());
- EXPECT_TRUE(rowset_meta->rowset_id() == _rowset->rowset_id());
- // FIXME(Drogon): these is wrong when not real tablet exist
- // EXPECT_EQ(rowset_meta->start_version(), 10);
- // EXPECT_EQ(rowset_meta->end_version(), 11);
+ st = RowsetMetaManager::get_rowset_meta(_meta, _tablet_uid, _rowset->rowset_id(), rowset_meta);
+ ASSERT_TRUE(st.ok()) << st;
+ EXPECT_EQ(rowset_meta->rowset_id(), _rowset->rowset_id());
+ EXPECT_FALSE(k_engine->pending_local_rowsets().contains(rowset_id));
+ EXPECT_EQ(rowset_meta->start_version(), 10);
Review Comment:
warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
EXPECT_EQ(rowset_meta->start_version(), 10);
^
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1372526327
##########
be/src/olap/olap_common.h:
##########
@@ -20,6 +20,7 @@
#include <gen_cpp/Types_types.h>
Review Comment:
warning: 'gen_cpp/Types_types.h' file not found [clang-diagnostic-error]
```cpp
#include <gen_cpp/Types_types.h>
^
```
##########
be/src/olap/olap_common.h:
##########
@@ -378,11 +379,16 @@
int64_t mi = 0;
int64_t lo = 0;
- void init(const std::string& rowset_id_str) {
+ void init(std::string_view rowset_id_str) {
// for new rowsetid its a 48 hex string
// if the len < 48, then it is an old format rowset id
- if (rowset_id_str.length() < 48) {
- int64_t high = std::stol(rowset_id_str, nullptr, 10);
+ if (rowset_id_str.length() < 48) [[unlikely]] {
Review Comment:
warning: 48 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
if (rowset_id_str.length() < 48) [[unlikely]] {
^
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1783211049
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1783290205
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.34 seconds
stream load tsv: 550 seconds loaded 74807831229 Bytes, about 129 MB/s
stream load json: 21 seconds loaded 2358488459 Bytes, about 107 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.8 seconds inserted 10000000 Rows, about 347K ops/s
storage size: 17162006518 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1387618793
##########
be/src/olap/storage_engine.cpp:
##########
@@ -1247,8 +1242,14 @@ Status StorageEngine::execute_task(EngineTask* task) {
// check whether any unused rowsets's id equal to rowset_id
bool StorageEngine::check_rowset_id_in_unused_rowsets(const RowsetId& rowset_id) {
std::lock_guard<std::mutex> lock(_gc_mutex);
- auto search = _unused_rowsets.find(rowset_id.to_string());
- return search != _unused_rowsets.end();
+ return _unused_rowsets.contains(rowset_id);
+}
+
+PendingRowsetGuard StorageEngine::add_pending_rowset(const RowsetWriterContext& ctx) {
Review Comment:
warning: method 'add_pending_rowset' can be made static [readability-convert-member-functions-to-static]
be/src/olap/storage_engine.h:155:
```diff
- PendingRowsetGuard add_pending_rowset(const RowsetWriterContext& ctx);
+ static PendingRowsetGuard add_pending_rowset(const RowsetWriterContext& ctx);
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "dataroaring (via GitHub)" <gi...@apache.org>.
dataroaring merged PR #25921:
URL: https://github.com/apache/doris/pull/25921
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801967841
TeamCity be ut coverage result:
Function Coverage: 36.74% (8401/22865)
Line Coverage: 29.26% (68133/232858)
Region Coverage: 27.90% (35216/126211)
Branch Coverage: 24.73% (18008/72822)
Coverage Report: http://coverage.selectdb-in.cc/coverage/c747ed4dc151cde47a44b7e4103124d59ad78683_c747ed4dc151cde47a44b7e4103124d59ad78683/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1386717540
##########
be/src/olap/rowset/pending_rowset_mixin.h:
##########
@@ -0,0 +1,54 @@
+// 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.
+
+#pragma once
+
+#include <concepts>
+
+#include "io/fs/file_system.h"
+#include "olap/rowset/pending_rowset_helper.h"
+#include "olap/rowset/rowset_writer.h"
+#include "olap/storage_engine.h"
+
+namespace doris {
+
+// Pending rowset guard mixin for RowsetWriter
+template <class T>
+ requires std::derived_from<T, RowsetWriter>
+class PendingRowsetMixin final : public T {
+public:
+ template <typename... Args>
+ PendingRowsetMixin(Args&&... args) : T(std::forward<Args>(args)...) {}
+
+ ~PendingRowsetMixin() override = default;
+
+ Status init(const RowsetWriterContext& rowset_writer_context) override {
+ auto st = T::init(rowset_writer_context);
+ if (st.ok() && (!rowset_writer_context.fs ||
+ rowset_writer_context.fs->type() == io::FileSystemType::LOCAL)) {
+ // Only local rowset should register pending rowset here
+ _guard = PendingRowsetGuard {rowset_writer_context.rowset_id,
+ StorageEngine::instance()->pending_local_rowsets()};
+ }
+ return st;
+ }
+
+private:
+ PendingRowsetGuard _guard;
Review Comment:
Add a comment for the guard.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "JackDrogon (via GitHub)" <gi...@apache.org>.
JackDrogon commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1372727588
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@ Status DataDir::load() {
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
+ if (tablet_paths.empty()) {
return;
}
- LOG(INFO) << "start to path gc by tablet schemahash.";
+ LOG(INFO) << "start to path gc by tablet, dir=" << _path;
+ // Use double pointer to avoid move elements after erase a element
+ auto forward = tablet_paths.begin();
+ auto backward = tablet_paths.end();
int counter = 0;
- for (const auto& path : _all_tablet_schemahash_paths) {
- ++counter;
+ do {
if (config::path_gc_check_step > 0 && counter % config::path_gc_check_step == 0) {
std::this_thread::sleep_for(
std::chrono::milliseconds(config::path_gc_check_step_interval_ms));
}
+ auto& path = *forward;
TTabletId tablet_id = -1;
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
Review Comment:
add comment for this decision condition
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@ Status DataDir::load() {
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
+ if (tablet_paths.empty()) {
return;
}
- LOG(INFO) << "start to path gc by tablet schemahash.";
+ LOG(INFO) << "start to path gc by tablet, dir=" << _path;
+ // Use double pointer to avoid move elements after erase a element
+ auto forward = tablet_paths.begin();
+ auto backward = tablet_paths.end();
int counter = 0;
- for (const auto& path : _all_tablet_schemahash_paths) {
- ++counter;
+ do {
if (config::path_gc_check_step > 0 && counter % config::path_gc_check_step == 0) {
std::this_thread::sleep_for(
std::chrono::milliseconds(config::path_gc_check_step_interval_ms));
}
+ auto& path = *forward;
TTabletId tablet_id = -1;
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- // should not happen, because already check it is a valid tablet schema hash path in previous step
- // so that log fatal here
- if (tablet_id < 1 || schema_hash < 1) {
- LOG(WARNING) << "invalid tablet id " << tablet_id << " or schema hash " << schema_hash
- << ", path=" << path;
- continue;
- }
- TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id);
- if (tablet != nullptr) {
- // could find the tablet, then skip check it
- continue;
- }
- std::string data_dir_path =
- io::Path(path).parent_path().parent_path().parent_path().parent_path();
- DataDir* data_dir = StorageEngine::instance()->get_store(data_dir_path);
- if (data_dir == nullptr) {
- LOG(WARNING) << "could not find data dir for tablet path " << path;
+ auto tablet = _tablet_manager->get_tablet(tablet_id);
+ if (!tablet) {
Review Comment:
use if initialize
##########
be/src/olap/data_dir.cpp:
##########
@@ -704,55 +690,115 @@ void DataDir::_perform_path_gc_by_rowsetid() {
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
continue;
}
- if (tablet_id > 0 && schema_hash > 0) {
- // tablet schema hash path or rowset file path
- // gc thread should get tablet include deleted tablet
- // or it will delete rowset file before tablet is garbage collected
+ bool exists;
Review Comment:
add more space for improving readability
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1783154783
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801817924
<details>
<summary>TPC-H test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'</summary>
```
Tpch sf100 test result on commit 2e41c52cb754209bb32848e2baa36c66c5689f9e, data reload: false
run tpch-sf100 query with default conf and session variables
q1 5268 5004 5056 5004
q2 364 194 198 194
q3 2085 2058 2028 2028
q4 1495 1442 1407 1407
q5 4156 4115 4117 4115
q6 254 131 131 131
q7 2055 1585 1616 1585
q8 2762 2763 2762 2762
q9 10369 10280 10286 10280
q10 3479 3583 3557 3557
q11 370 262 262 262
q12 453 296 289 289
q13 4506 4103 4111 4103
q14 327 302 293 293
q15 645 582 560 560
q16 683 618 602 602
q17 1155 1099 1078 1078
q18 7858 7504 7512 7504
q19 1678 1695 1693 1693
q20 616 349 348 348
q21 4946 4568 4558 4558
q22 543 437 418 418
Total cold run time: 56067 ms
Total hot run time: 52771 ms
run tpch-sf100 query with default conf and set session variable runtime_filter_mode=off
q1 5029 4942 4964 4942
q2 331 256 235 235
q3 4079 3969 3977 3969
q4 2778 2735 2755 2735
q5 6573 6508 6565 6508
q6 246 127 124 124
q7 3158 2698 2716 2698
q8 4824 4772 4873 4772
q9 17708 17743 17747 17743
q10 4130 4185 4168 4168
q11 731 683 656 656
q12 969 838 815 815
q13 4294 3877 3838 3838
q14 396 377 352 352
q15 628 553 571 553
q16 769 681 671 671
q17 4193 3978 3920 3920
q18 9456 9459 9339 9339
q19 1802 1794 1781 1781
q20 2354 2038 2048 2038
q21 8821 8780 8741 8741
q22 940 875 837 837
Total cold run time: 84209 ms
Total hot run time: 81435 ms
```
</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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1781061171
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1386732944
##########
be/src/olap/rowset/pending_rowset_helper.h:
##########
@@ -0,0 +1,87 @@
+// 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.
+
+#pragma once
+
+#include <mutex>
+#include <string>
+#include <unordered_set>
+
+#include "olap/olap_common.h"
+
+namespace doris {
+
+class PendingRowsetSet;
+
+// Developers must create `PendingRowsetGuard` before writing first row data to a local rowset, and
+// hold this `PendingRowsetGuard` during rowset construction before adding the rowset to tablet.
+class PendingRowsetGuard {
+public:
+ PendingRowsetGuard() = default;
+
+ // Remove guarded rowset id from `PendingRowsetSet` if it's initialized
+ ~PendingRowsetGuard();
+
+ // Initialize guard and add rowset id to `PendingRowsetSet`
+ explicit PendingRowsetGuard(const RowsetId& rowset_id, PendingRowsetSet& set);
+
+ PendingRowsetGuard(const PendingRowsetGuard&) = delete;
+ PendingRowsetGuard& operator=(const PendingRowsetGuard&) = delete;
+
+ PendingRowsetGuard(PendingRowsetGuard&& other) noexcept {
+ assert(!_pending_rowset_set || (_rowset_id == other._rowset_id &&
Review Comment:
Use CHECK instead. This a severe bug, restart the process will help to recover.
##########
be/src/olap/rowset/pending_rowset_helper.h:
##########
@@ -0,0 +1,87 @@
+// 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.
+
+#pragma once
+
+#include <mutex>
+#include <string>
+#include <unordered_set>
+
+#include "olap/olap_common.h"
+
+namespace doris {
+
+class PendingRowsetSet;
+
+// Developers must create `PendingRowsetGuard` before writing first row data to a local rowset, and
+// hold this `PendingRowsetGuard` during rowset construction before adding the rowset to tablet.
+class PendingRowsetGuard {
+public:
+ PendingRowsetGuard() = default;
+
+ // Remove guarded rowset id from `PendingRowsetSet` if it's initialized
+ ~PendingRowsetGuard();
+
+ // Initialize guard and add rowset id to `PendingRowsetSet`
+ explicit PendingRowsetGuard(const RowsetId& rowset_id, PendingRowsetSet& set);
+
+ PendingRowsetGuard(const PendingRowsetGuard&) = delete;
+ PendingRowsetGuard& operator=(const PendingRowsetGuard&) = delete;
+
+ PendingRowsetGuard(PendingRowsetGuard&& other) noexcept {
+ assert(!_pending_rowset_set || (_rowset_id == other._rowset_id &&
+ _pending_rowset_set == other._pending_rowset_set));
+ _rowset_id = other._rowset_id;
+ _pending_rowset_set = other._pending_rowset_set;
+ other._pending_rowset_set = nullptr;
+ }
+ PendingRowsetGuard& operator=(PendingRowsetGuard&& other) noexcept {
+ assert(!_pending_rowset_set ||
Review Comment:
ditto
##########
be/src/olap/storage_engine.cpp:
##########
@@ -1058,59 +1058,52 @@ void StorageEngine::_parse_default_rowset_type() {
}
void StorageEngine::start_delete_unused_rowset() {
- std::unordered_map<std::string, RowsetSharedPtr> unused_rowsets_copy;
+ std::vector<RowsetSharedPtr> unused_rowsets_copy;
{
std::lock_guard<std::mutex> lock(_gc_mutex);
for (auto it = _unused_rowsets.begin(); it != _unused_rowsets.end();) {
uint64_t now = UnixSeconds();
- if (it->second.use_count() == 1 && it->second->need_delete_file() &&
+ auto&& rs = it->second;
+ if (rs.use_count() == 1 && rs->need_delete_file() &&
// We delay the GC time of this rowset since it's maybe still needed, see #20732
- now > it->second->delayed_expired_timestamp()) {
- if (it->second->is_local()) {
- unused_rowsets_copy[it->first] = it->second;
- }
- // remote rowset data will be reclaimed by `remove_unused_remote_files`
+ now > rs->delayed_expired_timestamp()) {
evict_querying_rowset(it->second->rowset_id());
+ // remote rowset data will be reclaimed by `remove_unused_remote_files`
+ if (rs->is_local()) {
+ unused_rowsets_copy.push_back(std::move(rs));
+ }
Review Comment:
DCHECK + LOG(ERROR) for remote rowset.
##########
be/src/olap/storage_engine.cpp:
##########
@@ -1058,59 +1058,52 @@ void StorageEngine::_parse_default_rowset_type() {
}
void StorageEngine::start_delete_unused_rowset() {
- std::unordered_map<std::string, RowsetSharedPtr> unused_rowsets_copy;
+ std::vector<RowsetSharedPtr> unused_rowsets_copy;
Review Comment:
Consider reserve space.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803034352
<details>
<summary>TPC-H test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'</summary>
```
Tpch sf100 test result on commit c747ed4dc151cde47a44b7e4103124d59ad78683, data reload: false
run tpch-sf100 query with default conf and session variables
q1 21136 4935 4961 4935
q2 2530 176 171 171
q3 12219 2002 2002 2002
q4 12279 1350 1403 1350
q5 9733 4089 4020 4020
q6 249 134 130 130
q7 2051 1615 1606 1606
q8 11228 2695 2708 2695
q9 10309 10303 10175 10175
q10 9905 3556 3534 3534
q11 370 247 252 247
q12 451 289 288 288
q13 21817 4021 4051 4021
q14 322 293 283 283
q15 610 540 562 540
q16 694 613 600 600
q17 1128 1080 1064 1064
q18 7969 7471 7431 7431
q19 1730 1666 1679 1666
q20 563 355 347 347
q21 4901 4541 4490 4490
q22 521 412 415 412
Total cold run time: 132715 ms
Total hot run time: 52007 ms
run tpch-sf100 query with default conf and set session variable runtime_filter_mode=off
q1 4899 4939 4889 4889
q2 316 240 235 235
q3 3957 3928 3963 3928
q4 2727 2744 2731 2731
q5 6557 6495 6455 6455
q6 242 119 125 119
q7 3137 2738 2746 2738
q8 4741 4731 4775 4731
q9 17731 17794 17526 17526
q10 4101 4178 4138 4138
q11 738 624 643 624
q12 952 820 835 820
q13 4252 3885 3860 3860
q14 366 335 349 335
q15 643 520 568 520
q16 782 692 678 678
q17 3881 3838 3866 3838
q18 9474 9240 9244 9240
q19 1851 1740 1739 1739
q20 2355 2035 2026 2026
q21 8694 8767 8772 8767
q22 915 869 854 854
Total cold run time: 83311 ms
Total hot run time: 80791 ms
```
</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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1804965239
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803430192
TeamCity be ut coverage result:
Function Coverage: 36.77% (8407/22862)
Line Coverage: 29.27% (68148/232829)
Region Coverage: 27.90% (35211/126203)
Branch Coverage: 24.72% (18002/72826)
Coverage Report: http://coverage.selectdb-in.cc/coverage/03f554902a1d6a6c9431e4a4aa8b7dc0104dfe02_03f554902a1d6a6c9431e4a4aa8b7dc0104dfe02/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801836634
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.61 seconds
stream load tsv: 570 seconds loaded 74807831229 Bytes, about 125 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.9 seconds inserted 10000000 Rows, about 346K ops/s
storage size: 17162391423 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1386709764
##########
be/src/service/backend_service.cpp:
##########
@@ -187,6 +188,8 @@ void _ingest_binlog(IngestBinlogArg* arg) {
return;
}
RowsetId new_rowset_id = StorageEngine::instance()->next_rowset_id();
+ PendingRowsetGuard pending_rs_guard {new_rowset_id,
+ StorageEngine::instance()->pending_local_rowsets()};
Review Comment:
auto prg = StorageEngine::instance()->pending_local_rowsets()->add_pending_rowset(new_rs_id);
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1386724187
##########
be/src/olap/tablet.cpp:
##########
@@ -1971,66 +1945,54 @@ void Tablet::reset_compaction(CompactionType compaction_type) {
}
Status Tablet::create_initial_rowset(const int64_t req_version) {
- Status res = Status::OK();
if (req_version < 1) {
return Status::Error<CE_CMD_PARAMS_ERROR>(
"init version of tablet should at least 1. req.ver={}", req_version);
}
Version version(0, req_version);
RowsetSharedPtr new_rowset;
- do {
- // there is no data in init rowset, so overlapping info is unknown.
- std::unique_ptr<RowsetWriter> rs_writer;
- RowsetWriterContext context;
- context.version = version;
- context.rowset_state = VISIBLE;
- context.segments_overlap = OVERLAP_UNKNOWN;
- context.tablet_schema = tablet_schema();
- context.newest_write_timestamp = UnixSeconds();
-
- if (!(res = create_rowset_writer(context, &rs_writer)).ok()) {
- LOG(WARNING) << "failed to init rowset writer for tablet " << tablet_id();
- break;
- }
-
- if (!(res = rs_writer->flush()).ok()) {
- LOG(WARNING) << "failed to flush rowset writer for tablet " << tablet_id();
- break;
- }
-
- if (!(res = rs_writer->build(new_rowset)).ok()) {
- LOG(WARNING) << "failed to build rowset for tablet " << tablet_id();
- break;
- }
-
- if (!(res = add_rowset(new_rowset)).ok()) {
- LOG(WARNING) << "failed to add rowset for tablet " << tablet_id();
- break;
- }
- } while (false);
-
- // Unregister index and delete files(index and data) if failed
- if (!res.ok()) {
- LOG(WARNING) << "fail to create initial rowset. res=" << res << " version=" << req_version;
- StorageEngine::instance()->add_unused_rowset(new_rowset);
- return res;
- }
+ // there is no data in init rowset, so overlapping info is unknown.
+ RowsetWriterContext context;
+ context.version = version;
+ context.rowset_state = VISIBLE;
+ context.segments_overlap = OVERLAP_UNKNOWN;
+ context.tablet_schema = tablet_schema();
+ context.newest_write_timestamp = UnixSeconds();
+ auto rs_writer = DORIS_TRY(create_rowset_writer(context, false));
+ RETURN_IF_ERROR(rs_writer->flush());
+ RETURN_IF_ERROR(rs_writer->build(new_rowset));
+ RETURN_IF_ERROR(add_rowset(std::move(new_rowset)));
set_cumulative_layer_point(req_version + 1);
- return res;
+ return Status::OK();
}
-Status Tablet::create_vertical_rowset_writer(RowsetWriterContext& context,
- std::unique_ptr<RowsetWriter>* rowset_writer) {
+Result<std::unique_ptr<RowsetWriter>> Tablet::create_rowset_writer(RowsetWriterContext& context,
+ bool vertical) {
context.rowset_id = StorageEngine::instance()->next_rowset_id();
_init_context_common_fields(context);
- return RowsetFactory::create_rowset_writer(context, true, rowset_writer);
+ std::unique_ptr<RowsetWriter> rowset_writer;
+ if (auto st = RowsetFactory::create_rowset_writer(context, vertical, &rowset_writer); !st.ok())
+ [[unlikely]] {
+ return unexpected(std::move(st));
+ }
+ return rowset_writer;
}
-Status Tablet::create_rowset_writer(RowsetWriterContext& context,
- std::unique_ptr<RowsetWriter>* rowset_writer) {
+Result<std::unique_ptr<RowsetWriter>> Tablet::create_pending_rowset_writer(
+ RowsetWriterContext& context, bool vertical) {
context.rowset_id = StorageEngine::instance()->next_rowset_id();
_init_context_common_fields(context);
- return RowsetFactory::create_rowset_writer(context, false, rowset_writer);
+ std::unique_ptr<RowsetWriter> rowset_writer;
+ if (vertical) {
Review Comment:
consider
```
struct RsMix : rowstwriter {
rs_guard; // expand rs_guard life span
};
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801638812
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803524280
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.92 seconds
stream load tsv: 553 seconds loaded 74807831229 Bytes, about 129 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 33 seconds loaded 861443392 Bytes, about 24 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162535646 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1373053955
##########
be/src/olap/data_dir.cpp:
##########
@@ -627,74 +626,61 @@ Status DataDir::load() {
}
void DataDir::perform_path_gc() {
- std::unique_lock<std::mutex> lck(_check_path_mutex);
- _check_path_cv.wait(lck, [this] {
- return _stop_bg_worker || !_all_tablet_schemahash_paths.empty() ||
- !_all_check_paths.empty();
- });
- if (_stop_bg_worker) {
- return;
- }
-
- _perform_path_gc_by_tablet();
- _perform_path_gc_by_rowsetid();
+ auto tablet_paths = _perform_path_scan();
+ _perform_path_gc_by_tablet(tablet_paths);
+ _perform_path_gc_by_rowset(tablet_paths);
}
// gc unused tablet schemahash dir
-void DataDir::_perform_path_gc_by_tablet() {
- if (_all_tablet_schemahash_paths.empty()) {
+void DataDir::_perform_path_gc_by_tablet(std::vector<std::string>& tablet_paths) {
+ if (_stop_bg_worker) return;
+ if (tablet_paths.empty()) {
return;
}
- LOG(INFO) << "start to path gc by tablet schemahash.";
+ LOG(INFO) << "start to path gc by tablet, dir=" << _path;
+ // Use double pointer to avoid move elements after erase a element
+ auto forward = tablet_paths.begin();
+ auto backward = tablet_paths.end();
int counter = 0;
- for (const auto& path : _all_tablet_schemahash_paths) {
- ++counter;
+ do {
if (config::path_gc_check_step > 0 && counter % config::path_gc_check_step == 0) {
std::this_thread::sleep_for(
std::chrono::milliseconds(config::path_gc_check_step_interval_ms));
}
+ auto& path = *forward;
TTabletId tablet_id = -1;
TSchemaHash schema_hash = -1;
bool is_valid = _tablet_manager->get_tablet_id_and_schema_hash_from_path(path, &tablet_id,
&schema_hash);
- if (!is_valid) {
+ if (!is_valid || tablet_id < 1 || schema_hash < 1) [[unlikely]] {
LOG(WARNING) << "unknown path:" << path;
+ --backward;
+ std::swap(*forward, *backward);
continue;
}
- // should not happen, because already check it is a valid tablet schema hash path in previous step
- // so that log fatal here
- if (tablet_id < 1 || schema_hash < 1) {
- LOG(WARNING) << "invalid tablet id " << tablet_id << " or schema hash " << schema_hash
- << ", path=" << path;
- continue;
- }
- TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id);
- if (tablet != nullptr) {
- // could find the tablet, then skip check it
- continue;
- }
- std::string data_dir_path =
- io::Path(path).parent_path().parent_path().parent_path().parent_path();
- DataDir* data_dir = StorageEngine::instance()->get_store(data_dir_path);
- if (data_dir == nullptr) {
- LOG(WARNING) << "could not find data dir for tablet path " << path;
+ auto tablet = _tablet_manager->get_tablet(tablet_id);
+ if (!tablet) {
Review Comment:
done
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803331297
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803316213
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803429704
TeamCity be ut coverage result:
Function Coverage: 36.78% (8409/22862)
Line Coverage: 29.28% (68175/232829)
Region Coverage: 27.91% (35225/126203)
Branch Coverage: 24.73% (18010/72826)
Coverage Report: http://coverage.selectdb-in.cc/coverage/e52728cceced8e45529bd887594c66d40f3c43f8_e52728cceced8e45529bd887594c66d40f3c43f8/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1803535544
<details>
<summary>TPC-H test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'</summary>
```
Tpch sf100 test result on commit 03f554902a1d6a6c9431e4a4aa8b7dc0104dfe02, data reload: false
run tpch-sf100 query with default conf and session variables
q1 5170 5154 5203 5154
q2 369 188 196 188
q3 2089 2076 2040 2040
q4 1473 1447 1391 1391
q5 4100 4143 4076 4076
q6 251 131 132 131
q7 2061 1595 1603 1595
q8 2751 2707 2710 2707
q9 10312 10404 10186 10186
q10 3503 3570 3589 3570
q11 363 250 246 246
q12 471 298 297 297
q13 4511 4155 4068 4068
q14 313 291 284 284
q15 619 552 571 552
q16 713 626 602 602
q17 1125 1084 1080 1080
q18 7875 7417 7446 7417
q19 1679 1681 1668 1668
q20 596 389 353 353
q21 4922 4576 4584 4576
q22 522 449 439 439
Total cold run time: 55788 ms
Total hot run time: 52620 ms
run tpch-sf100 query with default conf and set session variable runtime_filter_mode=off
q1 4953 5015 5025 5015
q2 356 239 259 239
q3 3995 4006 3986 3986
q4 2798 2777 2770 2770
q5 6567 6451 6430 6430
q6 238 126 126 126
q7 3179 2673 2700 2673
q8 4732 4717 4731 4717
q9 17759 17720 17610 17610
q10 4128 4157 4186 4157
q11 824 638 649 638
q12 999 818 836 818
q13 4300 3873 3877 3873
q14 389 354 352 352
q15 637 559 574 559
q16 782 693 699 693
q17 3933 3898 3904 3898
q18 9649 9387 9482 9387
q19 1836 1757 1744 1744
q20 2360 2047 2040 2040
q21 8963 8796 8697 8697
q22 911 877 849 849
Total cold run time: 84288 ms
Total hot run time: 81271 ms
```
</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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "platoneko (via GitHub)" <gi...@apache.org>.
platoneko commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801929349
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1801782432
TeamCity be ut coverage result:
Function Coverage: 36.75% (8403/22866)
Line Coverage: 29.26% (68129/232856)
Region Coverage: 27.90% (35215/126212)
Branch Coverage: 24.73% (18006/72822)
Coverage Report: http://coverage.selectdb-in.cc/coverage/2e41c52cb754209bb32848e2baa36c66c5689f9e_2e41c52cb754209bb32848e2baa36c66c5689f9e/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1802011569
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.55 seconds
stream load tsv: 559 seconds loaded 74807831229 Bytes, about 127 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.2 seconds inserted 10000000 Rows, about 342K ops/s
storage size: 17162366859 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25921:
URL: https://github.com/apache/doris/pull/25921#discussion_r1387612079
##########
be/test/olap/rowset/pending_rowset_test.cpp:
##########
@@ -0,0 +1,44 @@
+// 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 <gtest/gtest.h>
+
+#include "olap/rowset/pending_rowset_helper.h"
+
+namespace doris {
+
+TEST(PendingRowsetTest, PendingRowsetGuard) {
+ PendingRowsetSet set;
+ RowsetId rowset_id;
+ rowset_id.init(123);
Review Comment:
warning: 123 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
rowset_id.init(123);
^
```
##########
be/src/olap/storage_engine.cpp:
##########
@@ -1247,8 +1242,14 @@ Status StorageEngine::execute_task(EngineTask* task) {
// check whether any unused rowsets's id equal to rowset_id
bool StorageEngine::check_rowset_id_in_unused_rowsets(const RowsetId& rowset_id) {
std::lock_guard<std::mutex> lock(_gc_mutex);
- auto search = _unused_rowsets.find(rowset_id.to_string());
- return search != _unused_rowsets.end();
+ return _unused_rowsets.contains(rowset_id);
+}
+
Review Comment:
warning: method 'add_pending_rowset' can be made static [readability-convert-member-functions-to-static]
be/src/olap/storage_engine.h:155:
```diff
- PendingRowsetGuard add_pending_rowset(const RowsetWriterContext& ctx);
+ static PendingRowsetGuard add_pending_rowset(const RowsetWriterContext& ctx);
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1781287095
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46 seconds
stream load tsv: 555 seconds loaded 74807831229 Bytes, about 128 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.9 seconds inserted 10000000 Rows, about 346K ops/s
storage size: 17162157363 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [feature](merge-cloud) Decouple rowset id generator and local rowsets gc implementation [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25921:
URL: https://github.com/apache/doris/pull/25921#issuecomment-1783290592
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.89 seconds
stream load tsv: 553 seconds loaded 74807831229 Bytes, about 129 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.2 seconds inserted 10000000 Rows, about 342K ops/s
storage size: 17161960830 Bytes
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org