SEZ9 commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6117104368
Thanks for the CI update on `452cd144` — CI is not the blocker here; the remaining items from my previous review are. To close this out: **Must-fix before I can approve** 1. **PR11781-F1 / PR11781-F3 (Test coverage of the legacy mappings)** – `WALReaderAndWriterTest` currently replays only the `JobStatus` mapping and asserts `getClass()`/`toString()`. Please extend it to cover all three legacy names (`JobStatus`, `JobResult`, `JobStatusData`) and assert on the actual deserialized value (enum identity / field values), not just the class name. For the two non-enum targets, please also add a compile-time check of the mapped target class names via the test-scoped `seatunnel-engine-common` dependency, so a future rename of the target classes fails the build instead of silently breaking the shim. 2. **PR11781-F2 (Diagnosability after rewrite)** – when the rewritten name still throws `ClassNotFoundException`, the error only reports the original WAL class name. Please include both the legacy name and the mapped target in the exception message (or log) so a stale/wrong mapping is diagnosable from the restart failure. **Nice-to-have / can be a follow-up** 3. **PR11781-F4 / PR11781-F5 (Docs)** – a short entry in `incompatible-changes.md` describing the #9689 relocation, that this shim restores reading of pre-relocation WAL files, and the downgrade semantics (WAL written by the new version is not readable by old versions) would help users on file-based IMap storage. 4. **PR11781-F6 (pom.xml)** – a one-line comment on the new test-scoped `seatunnel-engine-common` dependency explaining that production code intentionally references the class names as strings. 5. **PR11781-F7 (Fixture)** – if feasible, check in a small WAL frame produced by a pre-#9689 build, or document why generating it with the current `WALWriter` is equivalent. 6. **PR11781-F8 (Lookup design)** – either resolve the mapping transitively (loop until no further rewrite) or adjust the Javadoc so it does not suggest that simply extending the map is sufficient for a future relocation. Once items 1 and 2 are addressed, please ping me and I'll re-review promptly. If you'd rather split 3–6 into a follow-up PR, just say so and I'll track it separately. <!-- streview-comment:1692 --> -- 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]
