SEZ9 commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-5924175273
Thanks for the follow-up commit. I haven't been able to verify the new head (446ae12807) myself yet, so rather than guess at what changed since 835b264f3b, could you confirm the status of the earlier points? **Error reporting (F2)**: Does the `ClassNotFoundException` path in `WALReader` now report both the recorded WAL class name and the resolved target name, so a wrong or stale mapping entry is diagnosable? **Docs (F4, F5)**: If `incompatible-changes.md` (en and zh) is part of the change, does it record the #9689 relocation and the shim's upgrade/downgrade semantics for clusters using file-based IMap storage? **Test coverage (F1, F3)**: Does the regression test now cover all three legacy names (`JobResult`, `JobStatus`, `JobStatusData`) and assert deserialized field values rather than only `getClass()`/`toString()`? Since the target names are string literals the compiler cannot check, a test that resolves every mapping target against the real class on the classpath would close F3. **Legacy fixture (F7)**: Is the fixture still produced by the current `WALWriter`? A checked-in byte fixture, or at minimum a hand-built frame with the `org.apache.seatunnel.engine.core.job.*` name, would exercise what a pre-#9689 WAL actually contains. Not blocking if the frame format is unchanged, but please state that explicitly in the test. **Multi-hop lookup (F8)**: If the lookup is still a single-hop `getOrDefault`, a future relocation would require rewriting every existing target. Either loop until the name is stable (with a bound) or adjust the Javadoc so the "extend it" guidance matches single-hop behavior. **Pom comment (F6)**: A one-line comment on the test-scoped `seatunnel-engine-common` dependency explaining that production code intentionally references the class names as strings would help the next reader. None of these change the approach; the shim at the single `deserializeData` choke point is the right fix. Once the above are confirmed or pushed, I'm happy to merge. <!-- streview-comment:1447 --> -- 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]
