Doris-Breakwater commented on issue #68790: URL: https://github.com/apache/doris/issues/68790#issuecomment-6058464730
Breakwater-GitHub-Analysis-Slot: slot_bb7e27770ae5 The report identifies a valid FE defect and separate end-to-end gaps in S3 vault path-version support. A fix limited to the FE would still leave the requested layout unused; enabling only metadata propagation would expose cache and cleanup correctness risks. **Verification scope.** I inspected the local `4.1.4` tag at `ad35a140c7fd0b842f18c23300bac581f7d04326`, without modifying code. The captured issue is open and has no labels. The reported master commit `cfb47188bf` was unavailable locally and could not be fetched, so the conclusions below independently verify 4.1.4, not that master revision. This is source-level analysis plus a small Guava reproduction, not a running-cluster reproduction. **Verified findings in 4.1.4:** 1. **FE failure:** `CreateStorageVaultCommand` copies properties into `ImmutableMap`, then removes `path_version` / `shard_num` after integer parsing. For valid integer values, either property reaches an unsupported mutation once the preceding cloud-mode, privilege, and name checks pass. This happens before vault-type dispatch, so the same command path affects HDFS too. A standalone check with Guava `33.2.1-jre`, the release dependency, reproduced `UnsupportedOperationException` for both removals. Invalid numeric strings instead fail during parsing. [FE command](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStorageVaultCommand.java#L61-L135). 2. **Two independent S3 propagation losses:** after the FE failure is addressed, `StorageVault.fromCommand` retains the values and `StorageVaultMgr.buildAlterStorageVaultRequest` attaches `PathFormat` to the S3 request. However, meta-service's `ADD_S3_VAULT` branch constructs a new vault containing id, name, and object info without copying `path_format`. Independently, `CloudMetaMgr.get_storage_vault_info` supplies an empty `PathFormat` for S3, including named S3 vaults, while HDFS receives the stored format. The empty protobuf defaults to version 0. Thus fixing either loss alone is insufficient. [FE request construction](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/fe/fe-core/src/main/java/org/apache/doris/catalog/StorageVaultMgr.java#L186-L193), [S3 persistence](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/meta-service/meta_service_resource.cpp#L1535-L1542), [BE decoding](https://github.com/apache/doris/ blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/cloud/cloud_meta_mgr.cpp#L1844-L1859). 3. **The BE path mechanism already exists:** `CloudStorageEngine` passes the decoded format into `StorageResource`; remote rowset writers and readers use that resource's path methods. Version 1 generates the requested sharded segment layout using `murmur_hash64A` and the existing seed. Its shard function performs `% shard_num` without a positive-value check: zero becomes invalid when the function is invoked, rather than at vault construction. The inspected CREATE path also lacks supported-version and shard-range validation. In this tag the file is `be/src/storage/storage_policy.cpp`, rather than the older `io/fs` location. [Path implementation](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/storage/storage_policy.cpp#L159-L201). 4. **Conditional downstream hazards:** these are prerequisites for enabling version 1, not evidence that today's failing CREATE already caused corruption or leaks. - Both Doris-table cache reads and upload cache allocation hash the basename. Different rowsets' version-1 `0.dat` files therefore have identical cache identities; tablet id is not part of that hash. A reader-only fix would leave upload writes inconsistent. Cache eviction/statistics and peer-cache APIs also construct rowset-qualified version-0 names. [Reader](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/cached_remote_file_reader.cpp#L135-L142), [writer allocator](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/file_writer.h#L105-L129). - Recycler segment/index/delete-bitmap names and rowset/tablet prefixes assume version 0. Actual rowset and tablet deletion calls use those helpers, so unpacked version-1 objects would be missed. `checker.cpp` uses the same layout and could incorrectly report missing objects. Packed-file handling needs a separate audit; some packed delete-bitmap logic already matches logical ids. [Path helpers](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/util.h#L64-L93), [tablet deletion](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/recycler.cpp#L5019-L5027), [checker](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/checker.cpp#L638-L671). - The optional BE table-size correctness check also creates a default `StorageResource`, reconstructing version-0 paths instead of using the rowset's vault format. [Size checks](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/cloud/cloud_meta_mgr.cpp#L2349-L2416). **Recommended next steps for the proposed PR:** - Fix FE normalization using a mutable working copy while preserving its immutable property API. Return clear validation errors for malformed/unsupported versions and missing, zero, or negative shard counts with version 1; validate at the meta-service boundary as well. - Implement S3 persistence and BE decoding together with a vault-aware path contract across recycler/checker, segment and index files, delete bitmaps, and size checks. Reuse the existing hash definition, including seed and tablet-id representation. - Use one collision-free logical cache identity across readers, writers, warmup/peer APIs, eviction, and statistics. A rowset-qualified identity may preserve existing cache conventions; changing only basename hashing to full-path hashing needs a coordinated compatibility review. - Keep existing vaults and absent formats on version 0. Do not change an existing vault's layout in place without a migration design: reads reconstruct paths from the vault resource. Gate version-1 writes until all relevant BEs, meta-service, recycler, and checker support it. - Add regression coverage for each property alone and together; invalid values; persisted metadata and BE refresh/restart; two distinct rowsets/tablets with segment 0 and different contents under file cache; indexed and MoW tables; compaction/aborted loads and eventual DROP/TRUNCATE cleanup; and unchanged version-0 behavior. Existing `StorageResourceTest` covers path generation, but does not establish end-to-end vault support. **Information still needed:** please provide the complete FE exception stack and exact FE/BE/meta-service build SHAs. For the post-FE behavior, include the patch used, a redacted request/persisted vault format, and example resulting object keys. These establish the deployed path independently of the source analysis; credentials are unnecessary. The reported `UploadPart` SlowDown/503 failures remain a separate performance hypothesis. Sequential tablet ids and 23 Gbit/s alone do not prove S3 partition pressure or establish that 1,024 shards will eliminate it. AWS documents gradual request-rate scaling and possible transient 503s during scaling. To assess that link, provide timestamped BE upload errors with S3 request ids, requests/sec and concurrency, multipart part sizes/retry behavior, and key distribution before/after TRUNCATE. [AWS performance guidance](https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance.html). -- 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]
