Doris-Breakwater commented on issue #68970: URL: https://github.com/apache/doris/issues/68970#issuecomment-6123665866
Breakwater-GitHub-Analysis-Slot: slot_e23173589797 The code supports treating this as a BE/cloud S3 upload memory-bounding issue with high operational priority given the reported load cancellations and OOM pressure. It confirms the allocation and backlog mechanisms; it does not establish the contribution of each mechanism to the reported 210 GB peak or prove a memory leak. The proposed changes address relevant mechanisms, but the patch and end-to-end results are needed before judging correctness or effectiveness. I checked the local public `4.1.4` tag at `ad35a140c7fd0b842f18c23300bac581f7d04326`, and fetched the relevant files from public master at `af933bf5d75b421594ba6436f5ba4d665b282c12`. The buffer allocation, submission, upload/cache lifetime, and memtable-limiter behavior discussed below are present in both. This issue currently has no labels or existing comments. Suggested classification: bug, BE/cloud I/O, memory/backpressure. **Verified mechanisms** - `FileBuffer::PartData` allocates the full configured part size. `FileBufferBuilder::build` switches to the global `S3FileBuffer` tracker, and `Allocator::alloc` checks and charges the requested allocation before allocating. `S3FileWriter::appendv` builds a pending buffer on the first nonempty append, so small files reserve a full part. Even an empty file gets a buffer during close. See [buffer allocation and tracking](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/s3_file_bufferpool.cpp#L68-L96), [writer append/close](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/s3_file_writer.cpp#L283-L371), and [allocator](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/core/allocator.cpp#L306-L312). - Submission captures a shared buffer in the upload task, then the writer can allocate the next part without waiting for the previous upload. The upload pool sets thread limits but no queue limit; `ThreadPoolBuilder` defaults to `INT_MAX`. Thus thread limits bound concurrent execution, not retained buffer memory. Strictly, this is an effectively unbounded queue rather than a literally infinite one. Simply setting a small queue limit would reject submissions with `SERVICE_UNAVAILABLE`, not provide blocking backpressure. See [pool construction](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/runtime/exec_env_init.cpp#L286-L305) and [queue default and rejection behavior](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/threadpool.cpp#L429-L450). - The task retains the buffer through the file-cache copy. With `enable_flush_file_cache_async=true`, the completion callback signals the writer **before** the cache copy, but the upload worker still performs that copy while retaining the buffer. Consequently, writer completion is not buffer-memory release. Segment and index closes can also queue asynchronously while retaining pending buffers. See [upload/cache lifetime](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/s3_file_bufferpool.cpp#L148-L173). - The existing memtable limiter does react to process/system memory pressure and can wait; it is not wholly absent. However, its own accounting sums memtable states, excluding the separately tracked S3 buffers, and `handle_memtable_flush` returns early when load memtable usage is low. Existing per-table pending-memtable backpressure also does not directly cap retained upload bytes. See [limiter decisions](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/load/memtable/memtable_memory_limiter.cpp#L95-L126) and [early return](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/load/memtable/memtable_memory_limiter.cpp#L190-L204). The tracker covers download buffers and non-load uploads too, so its aggregate peak cannot identify the load-only breakdown. **Points to resolve in the proposed PR** 1. **Gate coverage:** adding a byte gate only at the bottom of `handle_memtable_flush` would miss its normal early-return path. It must run independently of the existing memtable threshold. Also, [LoadChannelMgr::add_batch](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/load/channel/load_channel_mgr.cpp#L161-L181) deliberately bypasses that function for high-priority loads. Define their policy explicitly, and verify both this path and `VTabletWriterV2::_write_memtable`. Trace actual locks and dependencies at every wait site; the stated stress test does not establish that all production paths can drain. 2. **Packed-file exemption:** the mutex serializes appends, but does not establish a bound on outstanding buffers. `mark_current_packed_file_for_upload_locked` moves the old writer to `_uploading_packed_files` and creates another current writer; current writers are also indexed by storage resource. Older packed writers can therefore coexist with new ones. See [rotation](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/packed_file_manager.cpp#L539-L577). Avoiding a permit wait under this mutex is sensible, but exempt packed output needs explicit byte accounting/backpressure or another demonstrated bound. Test repeated rotation with uploads stalled. 3. **Lifetime and accounting:** release permits and decrement bytes when memory is actually released, including after cache copying, not merely at the writer callback. These actions must remain safe after writer destruction and balance on allocation/growth failure, submit failure, cancellation, upload failure, and close failure. Define load attribution for segment/index writers, packed output and its direct-write fallback; exclude compaction/schema change consistently. Test cache-copy stalls separately from S3 stalls. 4. **Growth and mutable settings:** keep logical part capacity separate from allocated capacity and preserve the final buffer address before exposing the upload stream. `Allocator::realloc` checks/charges the entire new allocation before releasing the old one, so transient tracker usage and memory headroom can exceed the steady-state growth delta. Test that failure leaves accounting and ownership balanced. Use a consistent per-writer part-size snapshot in allocation, splitting, cache sizing and completion, coordinating with [#68910](https://github.com/apache/doris/issues/68910). Test limiter shrinking/disabling and waiter wakeups during runtime changes. A default initial size of 256 KiB changes allocation behavior; document that distinction from unchanged multipart boundaries and disabled-by-default limits. 5. **Bound claims and compatibility:** the part limiter bounds permitted parts, not filling buffers or buffers waiting for permits. A byte gate with a finite maximum wait is soft backpressure: already scheduled flushes can overshoot, and timeout can admit more work during a prolonged stall. Specify timeout behavior and retained-memory headroom. Preserve multipart contents, checksums, successful close/commit semantics and downloaded-buffer behavior. Coordinate scheduling/fairness with [#68903](https://github.com/apache/doris/issues/68903); load-only controls cannot bound the combined tracker if background writes remain unconstrained. **Evidence needed and next steps** - Please link the patch/PR and test output, and provide the deployed BE build hash, effective upload/close/flush thread settings, cache settings, packed-file settings, and process/cgroup limits. Include a minimal synthetic table/Arrow schema, load concurrency, batch sizes, tablet count and reproducible upload slowdown. - Capture a synchronized timeline of `S3FileBuffer` current/peak bytes, `s3_file_buffer_allocated`, upload-pool active/queued tasks, `s3_file_writer_async_close_queuing`/processing, memtable memory/pending flushes, S3 latency/retries, cache-write latency, RSS and cgroup `memory.current`/`memory.events`. Include the full `MEM_LIMIT_EXCEEDED` allocator message and cancellation log. Instrument load versus background/download bytes and filling/permit-wait/queued/upload/cache-copy states; those are needed to attribute the measured peak. - Run an end-to-end baseline/patched comparison with both 5 MiB and 64 MiB parts, small files and multipart files, packed files enabled/disabled, both cache-completion modes, background compaction, cancellation, and prolonged stalls. Report peak/current tracked bytes, RSS, throughput, close latency, timeout behavior and read-back correctness. The reported unit stress test is useful evidence for its modeled topology; it is not yet an end-to-end deadlock or OOM guarantee. This investigation was read-only; I did not run the unpublished patch or reproduce the production measurements. For interim mitigation, reducing admitted concurrent loads limits producers and therefore buffer demand. A smaller part size reduces each retained allocation, but does not bound their count. If evaluating 64 MiB → 5 MiB, first drain existing writers and validate file-cache block-size constraints; changing it while writers are active intersects #68910. Benchmark the throughput/request-count tradeoff before rollout. Increasing upload threads alone does not cap memory and may not help a saturated S3 endpoint or slow cache copy. -- 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]
