SEZ9 commented on PR #11781:
URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6136697464
Thanks @zhangshenghang for the rundown of `99827177`.
- **F1/F3**: a test that binds every `LEGACY_CLASS_NAME_MAPPINGS` target to
the live class via `JobStatus.class.getName()` etc. is exactly the compile-time
guard I was after, and the three-mapping coverage with deserialized-value
assertions from `452cd144` addresses the enum-only concern.
- **F2**: including both the original WAL name and
`resolveClassName(className)` in the `IMapStorageException` message is
sufficient for diagnosing a stale mapping target.
- **F4/F5**: the `incompatible-changes.md` (en/zh) entry with the
one-directional downgrade semantics covers both the upgrade note and the
incompatible-changes record.
- **F6**: the pom comment is what I asked for.
- **F8**: documenting resolution as single-level with one entry per
intermediate name is fine with me; no need for multi-hop resolution.
One note on numbering: your nice-to-have list is offset from my finding IDs
— your F3 covers my F4/F5, your F4 is my F6, and your F6 is my F8. I read your
F5 ("follow-up if still wanted") as my F7 (reading a WAL frame actually written
by a pre-relocation release rather than one produced by the current
`WALWriter`). Can you confirm that's what you meant? If so, a follow-up is fine
and it does not block this PR.
I'll go through the diff on `99827177` to confirm the above and then approve.
<!-- streview-comment:1702 -->
--
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]
