liaoxin01 opened a new pull request, #69694:
URL: https://github.com/apache/doris/pull/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.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Added `CheckEncryptionActionTest`. Both cases pass with this fix and fail
without it.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
Empty rowsets no longer fail the encryption check. A rowset that cannot be
checked now returns an error response instead of `"some are not encrypted"`.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
