DanielLeens commented on PR #11781: URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6310056067
Thanks @SEZ9, and sorry about the confusion. My previous comment was not cut off. It listed exactly three items under "Still open", and nothing else was pending: 1. The fixture is produced by the current `WALWriter` (acknowledged by the TODO at `WALReaderAndWriterTest.java:62-64`). 2. One bad frame aborts the whole restart (unchanged pre-existing fail-fast in `loadAllData`; I would keep it). 3. A Low nit about reflection and fully qualified names in `testLegacyMappingsTargetCurrentClassNames`. If the comment rendered truncated on your side, those three are the complete list. **Correction accepted on the mapping-target test.** "Fails the build" should have said "fails the unit-test job". `testLegacyMappingsTargetCurrentClassNames` compares the string values with `JobStatus.class.getName()` and friends, so a stale or mistyped target is caught when the test runs in CI, not at compile time. The strings stay literals on purpose, because production code must not depend on the old class locations. **On the fixture, here is the evidence for your second option (justify the gap explicitly).** I compared the three relocated types before and after the move, using the parent of `e7de54e4e7a` (#9689, the commit that moved them out of `seatunnel-engine-core`) against current `dev`: - `JobStatus`: the same 13 constants in the same order (`INITIALIZING` ... `UNKNOWABLE`), so the enum ordinals are unchanged. - `JobResult`: the same fields in the same order, `status` and `error`. - `JobStatusData`: the same fields in the same order and types, `jobId`, `jobName`, `jobStatus`, `submitTime`, `startTime`, `finishTime`. - The only structural difference is the `private static final long serialVersionUID = 1L` that #11257 added later. `ProtoStuffSerializer` uses `RuntimeSchema`, which skips static and transient fields, so it does not change the bytes. None of the three types has an `Object`-typed or abstract field, so no other class name is embedded in the value bytes either. That means a frame written by a pre-#9689 release differs from the frame the current `WALWriter` produces only in the `valueClassName` string of `IMapFileData`, and the legacy tests already set that string to the old name explicitly. This is a source-level comparison, not a replay of a real old WAL file. I did not run anything, and a captured byte fixture would still be the strongest proof if someone has an old WAL at hand. So I would suggest @zhangshenghang replaces the TODO in `WALReaderAndWriterTest` with a short comment stating this argument (identical field layout and enum ordinals before and after #9689, class name being the only difference on the wire), and puts the same two sentences in the PR description. That satisfies your second option without a new fixture. If a checked-in fixture is easy to obtain, it is a welcome follow-up, but I would not hold the merge for it. Nothing else has changed on my side: the head is still `f6869ab0`, the fork `Build` is green, and I have no source-level blocker. The PR still needs a write-capable maintainer to finish the approval. -- 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]
