Doris-Breakwater commented on issue #68508: URL: https://github.com/apache/doris/issues/68508#issuecomment-5825166237
## Initial maintainer analysis **Assessment: confirmed BE client-cache lifetime/design bug; keep this issue open.** The issue currently has no labels or assignee. I recommend triaging it as an object-storage/BE resource-lifetime bug and considering a 4.1 backport once the fix is proven. ### Verified facts - On the `4.1.4` tag, [`S3ClientConf` full-field equality and its hash include `bucket`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/s3_util.h#L68-L117), while URI conversion always copies the URI bucket into `client_conf.bucket` ([source](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/s3_util.cpp#L511-L518)). [`S3ClientFactory::create()`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/s3_util.cpp#L210-L248) only looks up and inserts into the process-wide singleton's `_cache`; there is no erase, capacity, TTL, or lifecycle reset. Therefore every distinct full configuration is retained until BE shutdown, and otherwise-identical configurations with different buckets necessarily create distinct entries. - Current `master` (`573c93c63b88fb68ac1d318dcd3e1672dd2361e6`) and current `branch-4.1` (`5e1ee245e23adedc0e9fff8b6006f067653e9155`) retain the same behavior. This is not limited to the reported release. - Each miss constructs a new `Aws::S3::S3Client` and credentials provider. Importantly, [`_create_s3_client()` does not consume `s3_conf.bucket`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/s3_util.cpp#L415-L463); bucket is supplied on each object request. The bucket is construction identity for Azure's container client, but not for the non-Azure S3-compatible client. This makes bucket-sensitive identity avoidable for the reported non-Azure case. - The code proves monotonically growing live ownership of client objects as new cache identities are seen. It does **not**, by itself, prove the reported ~700 MiB magnitude or the exact libcurl/OpenSSL/CA allocation breakdown. Those measurements are plausible and valuable reporter evidence, but the underlying heap-profile artifacts are still needed to independently verify per-entry cost and the dominant allocation stacks. ### Scope corrections - #68117 is currently an **open** PR, not behavior already present in `master` or 4.1. Its proposed Azure-only cache is a useful implementation reference (capacity-bounded LRU plus credential-expiry pruning), but it should not be described as an already-landed Doris cache. - The `maxConnections = 102400` fallback is real, but it is a ceiling, not an eager allocation of 102400 connections. It may amplify per-client transport use under concurrency, but it is separate from the cache-cardinality/lifetime defect and should be handled independently. - The credentials-provider statement should be narrowed. In 4.1.4, the v2 WebIdentity path does default-construct `STSAssumeRoleWebIdentityCredentialsProvider`, while explicit `role_arn` AssumeRole paths construct an STS client with Doris's selected CA configuration. Current `master` likewise passes `sts_client_config` to explicit AssumeRole, but still default-constructs the WebIdentity base provider. Thus the bypass concern applies to the WebIdentity-default path, not to every STS-style provider. ### Recommended fix direction 1. Introduce a **provider-specific cache key**. For non-Azure S3-compatible clients, omit `bucket`; retain it for Azure/container-bound clients. Do this in a cache-key type or normalization step rather than changing global `S3ClientConf::operator==`, which is also used by `ObjClientHolder` reset logic. This directly collapses bucket-per-table workloads to one client when transport and credential configuration are otherwise identical. 2. Bound the remaining non-Azure configuration space with capacity plus idle TTL/LRU eviction. Eviction should drop only the cache's `shared_ptr`, so active readers remain valid. Preserve construction outside the mutex, double-check on publication, and avoid destroying an evicted/losing AWS client while holding the factory lock. 3. Add low-cardinality observability: current entries, hits, misses, evictions, and client creations by provider. This will make the reported bucket-count/memory correlation testable in production. 4. Reassess a separate credentials-provider cache only after key normalization. Reusing one S3 client across buckets already reuses its provider; sharing providers across genuinely different/rotating token configurations has expiry and refresh semantics that need dedicated tests. Suggested tests: two non-Azure buckets with identical transport/credentials reuse one fake client; Azure buckets remain distinct; capacity and idle expiry evict the expected entry while an externally held `shared_ptr` remains usable; credential/token changes still create a new identity; concurrent misses publish one retained entry; and destructor counters prove evicted clients are released. ### Evidence requested to quantify impact and validate the regression Please attach, with credentials/endpoints/bucket names redacted: 1. Before/after jemalloc heap dumps (or `jeprof`/flamegraph outputs) for a known number of newly touched buckets, including resolved cumulative stacks for S3 client, curl, TLS/CA, and credential-provider allocations. 2. A synchronized time series of distinct buckets touched, `create one s3 client` log count, jemalloc `allocated`, RSS, and Doris `UntrackedMemory`; include a control that repeatedly scans already-seen buckets and the drop after BE restart. 3. The effective credential configuration (`aws_credentials_provider_version`, provider type, whether `role_arn` is set, and whether AK/SK/token values rotate per table) and effective `max_connections`, without secret values. In particular, confirm whether the per-table configurations differ only by bucket or also by temporary credentials. 4. The number of buckets/clients behind the reported ~700 MiB delta and the approximate bytes-per-new-client slope. These artifacts are not required to establish the unbounded-ownership bug, but they are needed to validate the claimed allocation sites, choose a safe default bound/TTL, and create a meaningful memory regression test. Breakwater-GitHub-Analysis-Slot: slot_2220f6d6b149 -- 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]
