github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4374759088
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,14 +1052,32 @@ int InstanceChecker::do_inverted_check() {
// so we choose to skip here.
TEST_SYNC_POINT_RETURN_WITH_VALUE("InstanceChecker::do_inverted_check",
(int)0);
- for (auto& [_, accessor] : accessor_map_) {
+ // Several resources may be backed by the same storage location, e.g.
storage vaults on the same
+ // bucket and prefix with different credentials. List each location only
once, otherwise its
+ // objects would be scanned and their leaks counted repeatedly. Compare
the location as
+ // configured, not the normalized `uri()`: prefixes "a/b" and "a//b" share
a uri but hold
+ // different objects, and skipping one would hide its leaks.
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
Review Comment:
[P2] Exclude nested vault namespaces from the parent scan
Two valid S3 vaults on the same endpoint/bucket can have prefixes `root` and
`root/data`: FE requires a nonempty root, and `create_object_info_with_encrypt`
preserves it without adding isolation. A healthy child-vault object
`root/data/data/10001/R_0.dat` is also returned by the parent's
`list_directory("data")`. After the parent strips `root/`, the checker receives
`data/data/10001/R_0.dat`; both file validators parse the second `data` as
tablet 0 and return `-1`. The child subsequently checks its live object
successfully, but `check_ret` remains `-1`, so every round fails on healthy
data regardless of accessor order. Equal-location deduplication cannot handle
this overlap. Partition overlapping vault subtrees so each object is validated
with its owning relative prefix, and add a healthy nested-prefix case.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -946,16 +940,19 @@ int InstanceChecker::do_inverted_check() {
return -1;
}
- if (tablet_rowsets_cache.tablet_id == tablet_id) {
- if (tablet_rowsets_cache.rowset_ids.contains(rowset_id)) {
- return 0;
- } else {
- LOG(WARNING) << "rowset not exists, key=" << obj_key;
- return -1;
- }
+ if (tablet_rowsets_cache.tablet_id == tablet_id &&
Review Comment:
[P2] Match rowset liveness to the scanned storage location
With two distinct vault locations A/B, commit rowset R of tablet T with
`resource_id=A`, then leave an extra `data/T/R_0.dat` copy in B. Both objects
pass: the full tablet read stores R without its resource, and this cache
accepts R in either accessor. Even a fresh lookup in B accepts A's metadata, so
the check returns success and counts no leak. The forward check and recycler
select `rs_meta.resource_id()` (`checker.cpp:687`, `recycler.cpp:3912`), so B's
copy is unreferenced and will not be removed when A's rowset is recycled. Carry
the scanned location into metadata validation and cache identity, accepting
alias resources only when they resolve to the same storage. Add a cross-vault
orphan-copy case; the new tests use disjoint tablets and miss it.
##########
cloud/src/recycler/s3_accessor.cpp:
##########
@@ -310,8 +310,12 @@ std::optional<S3Conf> S3Conf::from_obj_store_info(const
ObjectStoreInfoPB& obj_i
return s3_conf;
}
+// The location is endpoint, bucket and prefix as configured; `uri_`
normalizes them for display.
+// '\n' never occurs in an endpoint or a bucket name, so different confs never
collide.
S3Accessor::S3Accessor(S3Conf conf)
- : StorageVaultAccessor(AccessorType::S3), conf_(std::move(conf)) {}
+ : StorageVaultAccessor(AccessorType::S3,
+ conf.endpoint + '\n' + conf.bucket + '\n' +
conf.prefix),
Review Comment:
[P2] Distinguish S3 buckets selected by the credential context
Endpoint/bucket/prefix equality does not prove namespace equality for
supported S3-compatible gateways. [Ceph RGW
multi-tenancy](https://docs.ceph.com/en/quincy/radosgw/multitenancy/) permits
the same bucket name in different tenants and resolves an unqualified bucket
using the authenticated user's tenant. Two path-style S3 vaults can therefore
share `endpoint=http://rgw:7480`, `bucket=doris`, and `prefix=warehouse`, while
their different AK/SK select disjoint buckets. Vault creation permits both
configurations and the accessor supplies their separate credentials to the
clients, but these location strings are equal: the checker skips one tenant
entirely and can return success despite leaks there. Unlike the earlier
normalized-prefix collision, all three strings here are identical. Deduplicate
only when the effective namespace is proven equal, conservatively retaining
authentication-context distinctions, and add a tenant-aware client fixture.
##########
cloud/src/recycler/hdfs_accessor.cpp:
##########
@@ -325,11 +327,44 @@ class HdfsListIterator final : public ListIterator {
std::vector<DirEntries> level_dirs_;
};
-HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
- : StorageVaultAccessor(AccessorType::HDFS), info_(info),
prefix_(info.prefix()) {
- if (!prefix_.empty() && prefix_[0] != '/') {
- prefix_.insert(prefix_.begin(), '/');
+namespace {
+
+// Either empty or starts with '/', e.g. "p1" and "/p1" are the same directory
+std::string accessor_prefix(const HdfsVaultInfo& info) {
+ std::string prefix = info.prefix();
+ if (!prefix.empty() && prefix[0] != '/') {
+ prefix.insert(prefix.begin(), '/');
}
+ return prefix;
+}
+
+// fs_name and prefix name the directory, but a logical nameservice in fs_name
resolves through the
+// vault's hdfs_confs: two vaults with fs_name hdfs://ns and prefix /warehouse
are different
+// clusters when their dfs.namenode.rpc-address.ns.* differ. Append the
effective confs, sorted by
+// key, so equal locations mean equal configurations; differences that do not
matter only cost a
+// second listing. '\n' separates the parts, so e.g. hdfs://ns/a + /b and
hdfs://ns + /a/b differ.
+std::string accessor_location(const HdfsVaultInfo& info) {
+ // libhdfs applies builder confs so that the first value set for a key
wins: keep that one
+ std::map<std::string, std::string> confs;
+ for (const auto& conf : info.build_conf().hdfs_confs()) {
+ confs.emplace(conf.key(), conf.value());
Review Comment:
[P2] Preserve repeated-key semantics on native HDFS builds
Keeping the first value matches Hadoop libhdfs, but the supported
MacOS/native branch links libhdfs3 (`cloud/CMakeLists.txt:303-322`). The pinned
libhdfs3
[builder](https://github.com/apache/doris-thirdparty/blob/libhdfs3-v2.3.9/src/client/Hdfs.cpp#L563-L570)
applies each value immediately, and
[Config::set](https://github.com/apache/doris-thirdparty/blob/libhdfs3-v2.3.9/src/common/XmlConfig.h#L164-L170)
overwrites the key, so the last value wins. With
`enable_distinguish_hdfs_path=false`, create two HA vaults with equal
nameservice/prefix/configuration, then ALTER one vault's RPC addresses:
`meta_service_resource.cpp:1007-1010` appends the new entries. That native
accessor now reaches the new cluster, while this identity keeps the old
addresses and deduplicates it with the other vault, hiding leaks. This differs
from the earlier unique-key mapping omission. Preserve ordered entries or
follow the selected backend's precedence, and cover the native backend.
--
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]
