This is an automated email from the ASF dual-hosted git repository.
liaoxin01 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 92d4236508d [fix](cloud) Skip empty rowsets before resolving fs when
checking tablet encryption (#69694)
92d4236508d is described below
commit 92d4236508de6ee4c45a9187ec9ba826b3163efc
Author: Xin Liao <[email protected]>
AuthorDate: Wed Oct 28 14:23:07 2026 +0800
[fix](cloud) Skip empty rowsets before resolving fs when checking tablet
encryption (#69694)
### What problem does this PR solve?
Issue Number: None
Related PR: #68063
Problem Summary:
`/api/check_tablet_encryption` can report `"some are not encrypted"` for
an encrypted tablet that has an empty rowset filling a version hole.
This shows up in cloud mode when `skip_writing_empty_rowset_metadata` is
enabled.
There are two defects in `is_tablet_encrypted()`:
1. It resolves the rowset fs before it skips rowsets without segments.
When a load writes no data to a tablet, no rowset meta is written for
it. BE then fills the version hole with an empty rowset copied from the
previous rowset's meta. In cloud mode that previous rowset can be the
initial `[0-1]` rowset, whose resource id is empty. No storage resource
can be resolved for it, so the check fails with `failed to get fs for
rowset`, even though the rowset has no file to check.
2. The error is returned as `return st;` from a function that returns
`Result<bool>`. `Status` converts implicitly to `bool`, so the error
becomes the value `false`. The API then answers `"some are not
encrypted"` instead of reporting the failure.
This PR skips rowsets without segments before resolving the fs, and
returns the error as `ResultError`. `is_tablet_encrypted()` is now
declared in the header so it can be unit tested.
---
.../http/action/check_encryption_action.cpp | 12 ++-
.../service/http/action/check_encryption_action.h | 6 ++
.../service/http/check_encryption_action_test.cpp | 92 ++++++++++++++++++++++
3 files changed, 106 insertions(+), 4 deletions(-)
diff --git a/be/src/service/http/action/check_encryption_action.cpp
b/be/src/service/http/action/check_encryption_action.cpp
index 82d77e04f49..ca3b61ec85b 100644
--- a/be/src/service/http/action/check_encryption_action.cpp
+++ b/be/src/service/http/action/check_encryption_action.cpp
@@ -74,6 +74,11 @@ Result<bool> is_tablet_encrypted(const BaseTabletSPtr&
tablet) {
rs_meta->end_version() == 1) {
return;
}
+ // Must be checked before resolving the fs: an empty rowset has no
file to check, and a
+ // version hole rowset may carry a resource id that resolves to no
storage resource.
+ if (rs->num_segments() == 0) {
+ return;
+ }
// Must not be `physical_fs()`: packed rowsets keep their segments as
slices inside a
// shared object, and only the `PackedFileSystem` wrapper can resolve
a segment path to
// that slice. `fs()` is not an option either, since it decrypts and
would hide the
@@ -85,9 +90,6 @@ Result<bool> is_tablet_encrypted(const BaseTabletSPtr&
tablet) {
return;
}
- if (rs->num_segments() == 0) {
- return;
- }
auto maybe_seg_path = rs->segment(0).path();
if (!maybe_seg_path) {
st = std::move(maybe_seg_path.error());
@@ -141,7 +143,9 @@ Result<bool> is_tablet_encrypted(const BaseTabletSPtr&
tablet) {
if (st) {
return is_encrypted;
}
- return st;
+ // Must be wrapped: `Status` converts to `bool`, so a bare `return st;`
would report the
+ // error as "not encrypted".
+ return ResultError(std::move(st));
}
Result<std::string> get_last_encrypt_footer(const BaseTabletSPtr& tablet) {
diff --git a/be/src/service/http/action/check_encryption_action.h
b/be/src/service/http/action/check_encryption_action.h
index ae4ce978b16..32912ce07c4 100644
--- a/be/src/service/http/action/check_encryption_action.h
+++ b/be/src/service/http/action/check_encryption_action.h
@@ -18,12 +18,18 @@
#include <gen_cpp/FrontendService_types.h>
#include "cloud/cloud_tablet_mgr.h"
+#include "common/status.h"
#include "runtime/exec_env.h"
#include "service/http/http_handler_with_auth.h"
#include "service/http/http_request.h"
+#include "storage/tablet/tablet_fwd.h"
#include "storage/tablet/tablet_manager.h"
namespace doris {
+// Returns whether every non-empty rowset of `tablet` ends with the encryption
magic code, or an
+// error if any file could not be checked.
+Result<bool> is_tablet_encrypted(const BaseTabletSPtr& tablet);
+
class CheckEncryptionAction : public HttpHandlerWithAuth {
public:
explicit CheckEncryptionAction(ExecEnv* exec_env, TPrivilegeHier::type
hier,
diff --git a/be/test/service/http/check_encryption_action_test.cpp
b/be/test/service/http/check_encryption_action_test.cpp
new file mode 100644
index 00000000000..2767b27ce63
--- /dev/null
+++ b/be/test/service/http/check_encryption_action_test.cpp
@@ -0,0 +1,92 @@
+// 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 "service/http/action/check_encryption_action.h"
+
+#include <gen_cpp/olap_file.pb.h>
+#include <gtest/gtest.h>
+
+#include <memory>
+#include <mutex>
+#include <string>
+#include <vector>
+
+#include "cloud/cloud_storage_engine.h"
+#include "cloud/cloud_tablet.h"
+#include "storage/rowset/rowset.h"
+#include "storage/rowset/rowset_factory.h"
+#include "storage/rowset/rowset_meta.h"
+#include "storage/tablet/tablet_meta.h"
+
+namespace doris {
+
+class CheckEncryptionActionTest : public testing::Test {
+public:
+ CheckEncryptionActionTest() : _engine(CloudStorageEngine(EngineOptions
{})) {}
+
+ void SetUp() override {
+ auto tablet_meta = std::make_shared<TabletMeta>(
+ 1, 2, 15673, 15674, 4, 5, TTabletSchema(), 6,
+ std::unordered_map<uint32_t, uint32_t> {{7, 8}}, UniqueId(9,
10),
+ TTabletType::TABLET_TYPE_DISK, TCompressionType::LZ4F, 0,
false, std::nullopt,
+ "size_based", 1024, 1000, 3600, 5, 1,
TInvertedIndexFileStorageFormat::V2,
+ TEncryptionAlgorithm::AES256);
+ ASSERT_EQ(tablet_meta->encryption_algorithm(),
EncryptionAlgorithmPB::AES_256_CTR);
+ _tablet = std::make_shared<CloudTablet>(_engine,
std::move(tablet_meta));
+ }
+
+protected:
+ void add_rowset(Version version, int64_t num_segments, const std::string&
resource_id) {
+ auto rs_meta = std::make_shared<RowsetMeta>();
+ rs_meta->set_rowset_type(BETA_ROWSET);
+ rs_meta->set_version(version);
+ rs_meta->set_rowset_id(_engine.next_rowset_id());
+ rs_meta->set_num_segments(num_segments);
+ // A set resource id makes the rowset remote, so its fs comes from the
storage resource.
+ rs_meta->set_resource_id(resource_id);
+ RowsetSharedPtr rowset;
+ ASSERT_TRUE(RowsetFactory::create_rowset(nullptr, "", rs_meta,
&rowset).ok());
+ std::unique_lock wlock(_tablet->get_header_lock());
+ _tablet->add_rowsets({rowset}, false, wlock, false);
+ }
+
+ CloudStorageEngine _engine;
+ std::shared_ptr<CloudTablet> _tablet;
+};
+
+// A version hole is filled with an empty rowset copied from the previous
rowset's meta. In cloud
+// mode that may be the initial [0-1] rowset, whose resource id is empty, so
no fs can be resolved
+// for it. It has no file, so it must be skipped rather than fail the whole
check.
+TEST_F(CheckEncryptionActionTest, EmptyRowsetWithoutStorageResourceIsSkipped) {
+ add_rowset(Version(2, 2), 0, "");
+
+ auto res = is_tablet_encrypted(_tablet);
+ ASSERT_TRUE(res.has_value()) << res.error();
+ EXPECT_TRUE(res.value());
+}
+
+// A rowset whose files cannot be checked must surface the error, not be
reported as unencrypted.
+TEST_F(CheckEncryptionActionTest, UncheckableRowsetReportsError) {
+ add_rowset(Version(2, 2), 1, "not_exist_resource");
+
+ auto res = is_tablet_encrypted(_tablet);
+ ASSERT_FALSE(res.has_value()) << "unexpected value: " << res.value();
+ EXPECT_NE(res.error().to_string().find("failed to get fs for rowset"),
std::string::npos)
+ << res.error();
+}
+
+} // namespace doris
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
