Rangsh commented on issue #12266:
URL: https://github.com/apache/seatunnel/issues/12266#issuecomment-5854038311
## Claim + revised design (storage-first)
Hi @SEZ9 @DanielLeens @CryoThrust — thanks for the earlier discussion on
this follow-up.
As the issue author, I'll take this one and open a separate PR once we align
on the approach below. I agree with @DanielLeens: this should stay in design
until there is a **storage-level** way to verify selected keys without
retaining the full keyspace in heap. Benchmark-only chunking / adaptive heap
estimates / a parameterized full `loadAll` are not sufficient on the current
file-backed path, so I am **not** pursuing those.
@CryoThrust — thank you for offering to help. Your adaptive / chunked /
parameterized directions were a useful starting point; after Daniel's
clarification I'm steering this toward a narrow storage contract instead.
Collaboration on review/tests for that direction is still very welcome — please
say if you already have in-progress work on the same storage path so we don't
duplicate.
### Why the current API blocks the acceptance criteria
Today Hazelcast `IMap.loadAll(keys, …)` ends up in:
- `FileMapStore.loadAll(Collection keys)` → always `mapStorage.loadAll()` →
then filters in memory
- `IMapStorage` exposes only whole-map `loadAll()`
- so requesting one key or the full growth batch has the **same
retained-heap shape**: the complete map is materialized first
That matches the OOM we hit under `initialStoredJobCount=1000` in #12173,
and why mid-trial full reload had to be abandoned there.
Importantly, the file reader stack already has the selective primitive we
need:
- `WALReader.loadAllData(path, searchKeys)`
- `LatestMutationAccumulator` drops non-matching keys before retention
- `WALReaderAndWriterTest#shouldRetainOnlyRequestedKeysWhileScanning`
already pins the reader behavior
- `DefaultReader` streams record-by-record (it does not slurps the whole
file into one giant buffer)
The gap is that `IMapFileStorage` / `FileMapStore` never pass the requested
keys into that path (`IMapFileStorage.loadAll()` currently calls
`loadAllData(..., new HashSet<>())`, which is treated as “load everything”).
### Proposed approach
#### Phase A — storage contract (required first)
1. Add an additive keyed load on `IMapStorage`, with a **default**
implementation that preserves today's behavior for unknown implementers
(SPI-safe), e.g. `loadAll(Collection<Object> keys)` defaulting to full
`loadAll()` + filter.
2. Override in `IMapFileStorage` to call `WALReader.loadAllData(path, keys)`
for a non-empty key set (true selective retention).
3. Change `FileMapStore.loadAll(Collection keys)` to call the keyed storage
API instead of whole-map `loadAll()`.
4. Update `NoOpMapStorage` accordingly.
5. Tests must prove the contract, not only the happy path:
- many filler keys + few targets → only targets returned
- deterministic coverage that the file implementation uses the filtered
path (not full materialize-then-filter)
- an explicit memory/regression guard aimed at the
`initialStoredJobCount=1000` growth pressure shape (unit coverage + a
Diagnostics / local `-Xmx` smoke if needed)
**Precise memory claim (please hold me to this wording):**
Phase A does **not** claim “never touches the WAL”. The WAL is still scanned
sequentially (time remains O(WAL)). What changes is retained heap: from O(all
unique keys) down to O(|requested keys|). Transient per-record deserialize of
non-matching frames can still occur; skipping value deserialize for non-matches
can be a later optimization if still needed. I believe this is the
“materialize” bound Daniel asked for.
#### Phase B — benchmark harness (only after Phase A)
Once Phase A is green:
- Widen `IMapJobGrowthBenchmarkWorkload` durability sampling from one
representative key to the **full growth batch** via `IMap.loadAll(batchKeys,
true)` (or equivalent), asserting every batch key is durable from MapStore —
not only resident.
- Keep verification **outside** measured SingleShot time.
- Preserve the existing first empty-pressure + trial tear-down sampling
windows unless we find a safer mid-trial strategy that still respects the heap
budget.
Phase B can land in the same PR after Phase A tests, or as a tight follow-up
PR on top of A — either is fine with me; I’d slightly prefer one PR if A’s
tests already prove the memory bound.
### Explicit non-goals for the first PR
- Benchmark-layer chunk loops over today’s `FileMapStore.loadAll`
- Heuristic / adaptive heap estimates
- A “full loadAll” feature flag as a substitute for the keyed contract
- WAL indexing / random seek
- In-process writer reopen (unrelated)
### Ask before coding
Does this direction look right?
In particular:
1. OK to treat Phase A as “keyed load through existing `searchKeys`
retention” with the precise scan-vs-retain wording above?
2. Prefer `loadAll(Collection)` overload with SPI `default`, or a
differently named method?
3. OK to include Phase B in the same PR once A’s tests pass?
I will **not** start coding until this design is confirmed. After agreement
I’ll open the PR and link it here.
--
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]
