DanielLeens commented on PR #11781:
URL: https://github.com/apache/seatunnel/pull/11781#issuecomment-6305096513
Thanks @SEZ9 for the thorough checklist. I re-read it against the current
head `f6869ab09850` (unchanged since my last review), and most of the items are
already in place on this head. Mapping them so we can close what is closed and
isolate what is genuinely left:
**Already addressed on this head**
- *Cover all three legacy names and assert on the deserialized value, not
only the class:*
`WALReaderAndWriterTest#testReaderLoadsAllLegacyJobModelClassNames` (L301)
writes `JobStatus`, `JobResult` and `JobStatusData` frames with the old
`org.apache.seatunnel.engine.core.job.*` names and asserts the replayed values
(`status` name, `JobResult` status plus error text, and every `JobStatusData`
field: id, name, status, submit/start/finish time).
`testReaderLoadsJobStatusWrittenWithLegacyClassName` (L277) covers the
single-enum case.
- *Show the rewritten target name when `ClassNotFoundException` happens:*
`WALReader.deserializeData` (`WALReader.java:142-165`) now reports both the
recorded and the resolved class name, and the stray `{}`/`s%` placeholders were
fixed to `%s`. `testUnknownClassNameSurfacesInExceptionMessage` (L368) asserts
both names are present in the message.
- *Resolve every mapping target:*
`testLegacyMappingsTargetCurrentClassNames` (L259) compares each map value to
`JobStatus.class.getName()`, `JobResult.class.getName()` and
`JobStatusData.class.getName()`, so a renamed or mistyped target fails the
build. This is why `seatunnel-engine-common` is a test-scoped dependency, and
the `pom.xml` now carries a comment explaining that production code
deliberately references the old names as strings.
- *Single-hop lookup vs. the Javadoc:* the Javadoc on
`LEGACY_CLASS_NAME_MAPPINGS` (`WALReader.java:46-62`) now states resolution is
single-level and that every intermediate name needs its own entry, so it no
longer promises transitive resolution.
- *User-facing upgrade note:* both `incompatible-changes.md` files (en and
zh) gained the "File-based IMap storage: legacy job model class names in WAL
files are remapped on startup" entry, including the one-way semantics (a WAL
written by a current build cannot be read by an older release, so a downgrade
needs the persisted WAL cleared or rewritten).
**Still open, and my read on each**
1. *Fixture is produced by the current `WALWriter`.* This is acknowledged in
the TODO at `WALReaderAndWriterTest.java:62-64`. One technical point that
narrows the gap: `ProtoStuffSerializer` uses `RuntimeSchema` (field-number
based), so the value/key bytes do not embed any class name. The only place the
old name lives in a frame is the `valueClassName` string of `IMapFileData`,
which these tests set to the old name explicitly. A frame from a pre-relocation
release therefore differs from the test frame only if the field layout of
`JobStatus`/`JobResult`/`JobStatusData` changed since then, and that is exactly
what a checked-in byte fixture would prove. I agree a real fixture is the
strongest evidence, but I would call it a recommended follow-up, not a merge
blocker, because the failure mode it guards (a field-layout drift in these
three classes) is independent of this shim.
2. *One bad frame aborts the whole restart.* That is unchanged pre-existing
behavior (`ClassNotFoundException` is wrapped into `IMapStorageException` and
propagates through `loadAllData`). I would keep it: silently skipping a
job-status frame during restore would drop job state without a trace, which is
worse than a clear fail-fast with both class names in the message.
3. *Nit (Low):* `testLegacyMappingsTargetCurrentClassNames` reads the
private map through reflection and uses fully qualified
`java.lang.reflect.Field` / `java.util.Map` in the method body. The other tests
already prove the mapping through `loadAllData`, so this one is a duplicate
guard; if you prefer, a package-private accessor or dropping the reflection
would be cleaner. Not blocking.
On CI: the `Build` check on `f6869ab09850` is green (fork run 38504983404),
and nothing from my side blocks. I only have read-level review rights here, so
a write-capable maintainer needs to give the final approval. Thanks
@zhangshenghang for all the iterations on this one.
--
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]
