SEZ9 commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-5903298554
Thanks @nzw921rx and @dybyte for the approvals. Before I merge, I'd like to close out the open points from my earlier review — as far as I can tell from the thread, none of them have been addressed on `835b264f3b` yet. If some already were, please point me to where and I'll re-check. Remaining asks, in priority order: 1. **Test coverage of the legacy mapping** (`WALReaderAndWriterTest.java`): the regression test currently only asserts `getClass()`/`toString()` on the replayed enum and exercises just the `JobStatus` mapping. Please extend it to cover all three legacy names (`JobStatus`, `JobResult`, `JobStatusData`) and assert on the actual deserialized values, not just the class/string form. Otherwise the `JobResult`/`JobStatusData` target names remain unverified string literals the compiler can't check. 2. **Error reporting after rewrite** (`WALReader.java`): when the class lookup fails after a legacy-name rewrite, the exception still only reports the original WAL class name. Please include the rewritten target name too, so a wrong or stale mapping is diagnosable rather than just aborting the cluster restart with a misleading message. 3. **Docs**: please add a short user-facing note (and an entry in `incompatible-changes.md`) describing the #9689 relocation and what this shim does on upgrade/downgrade for clusters using file-based IMap storage. Javadoc alone won't reach operators. 4. **Minor**: a one-line comment on the new test-scoped `seatunnel-engine-common` dependency in `pom.xml` explaining why production code deliberately references the class names as strings; and, if feasible, consider making the lookup handle chained relocations (or adjust the "extend it" Javadoc guidance so it matches the single-hop behaviour). Ideally the fixture would also be a WAL frame captured from a pre-#9689 build rather than one produced by the current `WALWriter`, but I won't block on that one. Items 1 and 2 are what I'd like to see before merging; 3 and 4 can be follow-ups if you prefer, just let me know. Thanks for the patience on this. <!-- streview-comment:1426 --> -- 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]
