dybyte commented on PR #11465: URL: https://github.com/apache/seatunnel/pull/11465#issuecomment-5016143197
> @dybyte you're right, and I traced it through the code to confirm the reachability. Sharing what I found. > > **The cross-region path is real.** `IMapFileStorage.initialize` sets `region = String.valueOf(System.nanoTime())` per instance and writes under `businessRootPath + region + "/"`, but `businessRootPath` is only `namespace/clusterName/businessName` — it does **not** include the region. `WALReader.loadAllData`/`loadAllKeys` read from `businessRootPath` and merge records across **all** region directories before sorting. Since `FileMapStoreFactory` uses the stable `mapName` as `businessName` while each `FileMapStore.init` creates a fresh `IMapFileStorage` (hence a fresh region), the same logical key can end up in different region dirs across **restart** or **ownership migration** on shared storage (OSS/HDFS). So a per-writer `AtomicLong` starting at 0 is not comparable across regions — exactly your point. > > **Why it corrupts the winner, concretely.** In the comparator the equal-timestamp chain is `sequenceId desc → deleted → keyClass → keyBytes → valueClass → valueBytes`. Two same-key records from different regions in the same millisecond will very likely carry equal small sequence ids, so `sequenceCompare == 0` and the tie-break sinks to `deleted` and then **value bytes** — i.e. content order, not append order. At that point last-write-wins is lost: a same-ms `delete`+`put` can resolve to the wrong survivor regardless of real order. This confirms your observation that `valueClassName`/value bytes don't represent replay order. > > **Scope note (being honest about likelihood).** Triggering an actual wrong state still needs the narrow window of a same-key write in the **same millisecond** across two regions. The correctness gap is real; the trigger is a low-probability boundary (restart/migration timing), not a common path. > > **On the fix.** Your key-identity-first ordering with a cross-writer-comparable version (ownership epoch + writer-local sequence) is the correct general solution, but it's a larger change (persisting an epoch, touching `IMapFileData` layout + serialization compatibility). A smaller alternative is to keep the per-writer sequence for the single-writer case and explicitly document that imap-file storage does not guarantee last-write for same-key same-millisecond writes across regions. **Which direction do you and the maintainers prefer** — the full epoch-based ordering, or the minimal fix + documented limitation? Happy to implement whichever you'd like to land. @nielifeng I think either approach is reasonable. -- 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]
