github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4355361062
##########
cloud/src/recycler/hdfs_accessor.cpp:
##########
@@ -325,11 +325,24 @@ 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;
+}
+
+} // namespace
+
+HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
+ : StorageVaultAccessor(AccessorType::HDFS,
+ info.build_conf().fs_name() +
accessor_prefix(info)),
Review Comment:
[P2] Include nameservice resolution in the HDFS location identity
With the supported enable_distinguish_hdfs_path=false setting, two HA vaults
can both use fs_name=hdfs://ns and prefix=/warehouse while their per-vault
dfs.namenode.rpc-address.ns.nn1/nn2 settings point to different clusters.
CREATE preserves those settings, and HDFSBuilder applies them to separate
forced-new connections, but both locations here become hdfs://ns/warehouse. The
checker consequently skips one namespace and can return success despite leaks
there. Include namespace-resolving configuration in the identity, or avoid
deduplicating accessors whose configurations do not prove equivalence, and
cover equal logical names with different address mappings. This is distinct
from the fixed empty/shadowed URI issue: both locations are populated but
resolve differently.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -950,12 +944,16 @@ int InstanceChecker::do_inverted_check() {
if (tablet_rowsets_cache.rowset_ids.contains(rowset_id)) {
return 0;
} else {
- LOG(WARNING) << "rowset not exists, key=" << obj_key;
- return -1;
+ // Garbage data leak, same as a cache miss below
+ LOG(WARNING) << "rowset should be recycled, key=" << obj_key;
+ return 1;
Review Comment:
[P2] Revalidate a negative cache lookup before counting a leak
A successful tablet read can become stale while this scan runs. For example,
after the first S3 page loads tablet T's existing rowsets into the cache, an
ordinary load can upload and fully commit a new rowset of T whose segment sorts
into a later page. The lazy listing then yields that live segment, but this
branch never refreshes metadata: it now returns 1 and increments
num_inverted_check_failed instead of the previous temporary error. The checker
lease does not stop ingestion, and there is no file-age boundary here. Recheck
a cached absence in a fresh transaction before declaring a leak, or establish a
consistent age/snapshot boundary. This also needs an interleaving test;
completing the initial read successfully does not prevent it.
--
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]
