Doris-Breakwater commented on issue #67062: URL: https://github.com/apache/doris/issues/67062#issuecomment-5392323800
## Initial maintainer analysis **Triage:** This is a valid BE spill-placement bug by code inspection, with potential performance and per-disk capacity impact. I checked current upstream `master` at `87a7f9c0be237fd180822bce4ca9077b9ac1e332`; the reported selection path is still present. The issue currently has no labels; it should be classified as a bug and routed to the BE spill owners. ### Verified facts - `_get_stores_for_spill()` filters candidates by storage medium and capacity, sorts them by `_get_disk_usage(0)`, and `create_spill_file()` always selects `data_dirs.front()`. - `_get_disk_usage()` uses only `_disk_capacity_bytes` and `_available_bytes`. Those values are refreshed by `update_capacity()` during initialization and then by the spill GC loop, whose default interval is 2,000 ms. - Successful writes update `_spill_data_bytes` immediately, but that counter is used only for the spill-data limit check and metrics; it does not change placement ordering. - Consequently, repeated file creations between capacity refreshes see the same ordering and select the same minimum-usage directory. This confirms the reported batching behavior; after a refresh, the whole batch may switch to another directory rather than being balanced within the interval. - The conclusion applies among spill paths with the same `storage_medium`. The current code intentionally uses eligible SSD paths first and considers HDD paths only when no SSD path is available, so mixed SSD/HDD paths are not expected to be balanced together. ### PR guidance The proposed direction is sound: retain the primary preference for genuinely emptier disks, but use a manager-owned random or round-robin tie-breaker among candidates whose effective usage is indistinguishable. A deterministic round-robin or injectable chooser would make the unit test non-flaky. Two implementation details need care: 1. Exact floating-point equality may not balance disks with slightly different free-space snapshots. The PR should define and test a small, documented equivalence tolerance while ensuring a clearly emptier disk still wins. 2. Simply adding all of `_spill_data_bytes` to the filesystem-reported used bytes can double-count spill data already reflected by the last `get_space_info()` refresh, and it still cannot reserve space for a burst of files created before their first write. A tie-breaker/reservation at file-assignment time is therefore still needed. There is also an adjacent concurrency issue to address in the PR: `_disk_capacity_bytes` and `_available_bytes` are documented as protected by `SpillDataDir::_mutex`, but `_get_stores_for_spill()` calls `_get_disk_usage(0)` after `reach_capacity_limit(0)` has released that mutex. The GC thread can update those fields concurrently. The capacity check and usage snapshot should be exposed through a lock-protected helper; any round-robin state used by concurrent creators must likewise be atomic or protected. Recommended coverage: - Multiple same-medium, equally empty directories: a burst of file assignments before a capacity refresh must use more than one directory. - Slightly different usage inside the chosen equivalence tolerance, plus a clearly emptier directory outside it. - Capacity-limited directories remain excluded and SSD-before-HDD behavior remains unchanged. - Concurrent creators, preferably with deterministic assertions rather than a probabilistic distribution test. ### Additional information The code is sufficient to confirm the placement defect, so logs or a query profile are not blockers for triage. To quantify the real-world impact and validate the eventual fix, it would still help to attach the exact BE commit SHA, the spill path configuration including each path's storage medium and physical mount, `spill_gc_interval_ms`/`spill_storage_limit`, and a short per-path time series of `spill_disk_avail_capacity` and `spill_disk_data_size`. BE warnings about spill or disk capacity limits would also clarify whether the observed early failure is the filesystem flood-stage limit or the configured spill-data limit. Given that the reporter is willing to submit a PR, the next step is a focused BE fix with the deterministic multi-directory tests above; a query profile is not required to start that work. Breakwater-GitHub-Analysis-Slot: slot_38163ea4ce6a -- 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]
