SEZ9 commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6306793003
Thanks for the detailed mapping of the checklist against `f6869ab09850`. The description lines up with what I asked for on the test coverage (all three legacy names with value assertions), the `ClassNotFoundException` message showing both the recorded and resolved class names, the mapping-target test, the `pom.xml` comment, the single-level Javadoc on `LEGACY_CLASS_NAME_MAPPINGS`, and the `incompatible-changes.md` entries. I have not re-walked the diff for each of those yet, so I will confirm them against the actual changes rather than close them from the summary alone; if anything there is still in progress rather than already on this head, please say so. One small correction on the mapping-target test: a comparison of string literals against `JobStatus.class.getName()` and friends catches a stale target at test time, not at compile time. That is still a good improvement over an unverified literal, I just want the description to be precise about it. On the fixture point (the TODO acknowledging the frames are produced by the current `WALWriter`): I don't want to block on this, but before merge I'd like one of: - a small checked-in WAL file (or hard-coded byte array) captured from a pre-#9689 build and replayed by a test; or - if the argument is that `ProtoStuffSerializer`'s `RuntimeSchema` is field-number based and the class-name string is therefore the only thing that differs on the wire, please spell that out in the test comment and the PR description so the gap is explicitly justified rather than left as a TODO. Your last comment looks cut off partway through item 1 of the "still open" list. If there were further open items, could you post them so I can review everything in one pass? <!-- streview-comment:1911 --> -- 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]
