matanper commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4376318440
##########
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:
Fixed in 22b5e5a254a, root made alias-safe in a7ce4faabe7. Each accessor now
has a `StorageRoot` (the S3 bucket, empty for HDFS, plus the prefix without
empty path components). When another vault's root is inside the listed `data/`
of the vault being scanned (e.g. `root/data` inside `root`), the parent skips
that `data/.../data/` subtree; those objects are checked once, when the child
vault is listed. Test: `InvertedCheckTest.nested_vault`.
##########
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:
Fixed in 22b5e5a254a, root made alias-safe in a7ce4faabe7. The tablet cache
now keeps `rowset_id -> resource_id`, and a segment is only covered by its
rowset if the rowset's vault has the root it was listed from; otherwise it is a
leak (a cache hit that fails the check re-reads the meta first, as for a miss).
Ownership compares roots, not listing locations, and the root is built to err
toward "same": it is the bucket (empty for HDFS) plus the prefix without empty
path components, with no endpoint/fs_name since those have aliases (http/https,
internal/public endpoints; host/FQDN/IP/nameservice). So a different root
surely means different paths, and vaults that may be the same storage (other
credentials, an endpoint alias) accept each other's objects: an ambiguity can
only hide a copy, never report live data. A resource with no accessor (an HDFS
vault in a build without HDFS) is trusted. Test:
`InvertedCheckTest.segment_in_another_vault_is_leak`.
##########
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:
Fixed in 22b5e5a254a: the location keeps all `hdfs_confs` in their
configured order instead of picking a winner, so identical locations mean
identical configurations under either precedence. A reordering now only costs
an extra listing (the root, and so ownership, ignores confs). Test:
`HdfsAccessorLocationTest.location`.
##########
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:
Fixed in 22b5e5a254a: the S3 location adds provider, ak, role_arn,
external_id and the credential provider type (never the sk), so vaults that
differ only in identity are listed separately. The root stays
endpoint/bucket/prefix, so the duplicate listing does not turn into false
leaks. Test: `S3AccessorLocationTest.location_includes_the_credential_identity`.
--
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]
