SEZ9 commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6282264699
Thanks for the update. With CI reported passing on `f6869ab09850` (run 38504983404), there is no build-side blocker from my side. My earlier review points are still open, though. Here is what I'd still like to see before approval: **Tests (`WALReaderAndWriterTest.java`)** - The regression test only asserts `getClass()`/`toString()` on the replayed enum and covers one of the three legacy mappings. Please cover all three legacy names and assert on the deserialized value, not just the class. - The legacy fixture is produced by the current `WALWriter`, so the test never reads a frame actually written by a release that used the old class names. A checked-in byte fixture or a hand-built frame with the old class name would make this a real compatibility test. **Robustness (`WALReader.java`)** - When `ClassNotFoundException` is thrown after the legacy-name rewrite, the error still reports only the original WAL class name. Please include the rewritten target name so a wrong or stale mapping is diagnosable, and consider whether a single bad frame should abort the whole cluster restart. - The `JobResult` and `JobStatusData` target names are unverified string literals. Since `seatunnel-engine-common` is already a test-scoped dependency, a test that resolves every mapping target would catch typos. - The single-hop `getOrDefault` lookup means a future relocation requires rewriting every existing target, which contradicts the "extend it" guidance in the Javadoc. Either resolve mappings transitively or adjust the Javadoc. **Docs** - Please add a short user-facing upgrade note for clusters using file-based IMap storage, and record the class relocation plus this shim's upgrade/downgrade semantics in `incompatible-changes.md`. **Style (`pom.xml`)** - A one-line comment on the test-scoped `seatunnel-engine-common` dependency explaining why production code deliberately references class names as strings would help future readers. If you've decided against any of these, a short reply is fine and we can close them out. Happy to re-review once a new push lands. <!-- streview-comment:1893 --> -- 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]
