SEPURI-SAI-KRISHNA commented on PR #29345: URL: https://github.com/apache/flink/pull/29345#issuecomment-6194480123
Thanks, all twelve are addressed and I have replied on each thread. Two were real bugs and both have a test that fails without the fix. The `Row.copy` one went further than the output rows. The state map keys, the pending timer map keys and `Timer`'s own key had it too, and there it is worse because `Row` hashes deeply. Everything deep copies through `ExternalSerializer` now. Checking the isolation claim in the other direction turned up a gap of my own. Every test mutated the live harness and asserted the snapshot held, and nothing asserted the reverse. Restoring one snapshot twice shared the per-key state maps, the `Timer` objects and the rows in the output lists. Deleting the copies on the restore side left the whole suite green, so none of it was covered. Four tests cover it now. One change you did not ask for, so flagging it rather than leaving it in the diff: `snapshot()` now requires the harness to be open, matching `process` and `getOutput`, so snapshotting a closed harness is illegal where it previously worked. Say if you would rather it stayed permissive. Left alone on purpose: `getOutput()` and `getFunctionOutput()` use `List.copyOf`, which throws on a collected null, so `collect(null)` plus either getter fails on master today, independent of snapshotting. Happy to file that separately. Open in the threads: `MapViewStateConverter`, cloning the arrays in `HarnessSpec`, the `build(TestSnapshot)` name, and inlining `Timer.copy`. Happy to take any of them as follow-up. 154 tests pass, spotless and checkstyle clean. -- 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]
