SEZ9 commented on issue #12266: URL: https://github.com/apache/seatunnel/issues/12266#issuecomment-5882283388
Thanks @Rangsh — that fills in the heap context. - **4g fork budget**: understood that the growth workload's JMH fork JVM args (`-Xms4g` / `-Xmx4g`, G1 + `AlwaysPreTouch`) in `IMapJobStorageBenchmark` are what the Benchmarks / Diagnostics workflows run with, and that nothing overrides them. Treating "must not OOM under the 4g fork at `initialStoredJobCount=1000`" as the acceptance bar for the full-batch reload is the right framing. - **Earlier OOM**: the Diagnostics run `34317023975` on `432bdb3d9` failing in `WALReader.loadAllData` / `FileMapStore.loadAll` on the first-iteration full-batch reload is a sufficient reproduction reference for this issue. No need to recover a peak-heap capture from that failure; a constrained-heap run once the keyed path exists will be more useful evidence. - **`5bbc304b2` + local `-Xmx2g`**: noted as the current single-key baseline. To be explicit, that smoke shows the narrowed sample fits, not that a full-batch reload will — so it should not be cited as evidence for Phase B. On your two commitments, both sound right: 1. Phase A: keyed file-storage contract proven by deterministic `FileMapStore` / WAL reader tests (filler keys, overwrite/tombstone cases, no unrelated keys returned, no whole-map retention), with the constrained-heap diagnostic as supporting evidence only. Please run that diagnostic against the same `initialStoredJobCount=1000` shape that OOM'd, so it is a like-for-like regression guard. 2. Phase B: durability sample only from iteration / trial tear-down hooks, with an explicit assertion that it sits outside the measured `SingleShot` path — please include that assertion in the initial revision rather than as a follow-up. As you already agreed, this issue stays design-only until the parent implementation is healthy and merged, with no stacked benchmark or storage PR before then. When you open Phase A, please link the constrained-heap Diagnostics run alongside the deterministic tests so reviewers can see both the functional contract and the 4g-budget evidence together; Phase B should reference the merged Phase A change. Nothing else outstanding from my side. Thanks for the thorough follow-up. <!-- streview-comment:1384 --> -- 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]
