DanielLeens commented on PR #12466:
URL: https://github.com/apache/seatunnel/pull/12466#issuecomment-5846308341
Thanks for digging this deep — I re-pulled this head and walked through all
five points against the actual source, and every one of them checks out. Going
through them:
**Issue 1 (test lifecycle, Medium)** — agreed, and I'd like to see this
fixed before merge since it's cheap and directly prevents confusion on the very
next slice. `testDeclaredSerialVersionUidsMatchPreviouslyGeneratedValues` and
the two `Reads...PreviousWireForm` tests are the permanent guard (old worker →
new master must keep working) and must survive a field addition. The two
`Writes...PreviousWireForm` tests are, as you say, a one-off proof that *this*
PR didn't change anything — they pin the whole class descriptor via raw byte
equality, so they will (correctly) go red the moment STIP-33 adds the optional
field, and whoever's doing that slice should not "fix" that by just
regenerating the fixtures, since that would silently drop the
reader-compatibility check that actually matters. Could you extend the class
Javadoc to spell that lifecycle out explicitly, plus the one-line
fixture-regeneration recipe? I'd also support the rename to something like
`...ExactBytesUnchangedB
yUidDeclaration` — it makes the intent self-evident without anyone having to
read the Javadoc first.
**Issue 2 (Java-serialization proxy vs. Hazelcast wire, Low)** — confirmed:
`TaskExecutionState`/`TaskDeployState` aren't `IdentifiedDataSerializable`, and
`NotifyTaskStatusOperation`/`DeployTaskOperation` hand them to Hazelcast's
`ObjectDataOutput.writeObject`, which only falls back to plain Java
serialization because of that. So the test's bare `ObjectOutputStream` is a
valid proxy today, but it's a proxy, not the real path. Given the Low severity,
I'd rather see this captured as a one-line Javadoc assumption ("valid as long
as these classes stay plain `Serializable` with no custom Hazelcast serializer
registered") than have the author build out a full
`DefaultSerializationServiceBuilder` round-trip in this PR — that's real added
complexity for a guard that's already backed by the UID + descriptor pinning.
Happy to have that be a fast-follow instead of a blocker here.
**Issue 3 (nullability contract, Low)** — good catch, and this one's worth
fixing now too: the `Throwable` constructor already writes `""` for a null
throwable while the two-arg constructor leaves `throwableMsg = null`, so the
two producer paths don't even agree with each other today. Since serialization
bypasses constructors entirely, any field STIP-33 adds under this pinned UID
will show up as `null` (not a constructor default) on an old-writer/new-reader
pair. A short comment next to both `serialVersionUID` declarations stating
"fields added later must be nullable and read with a null-safe default; do not
bump this value" would make that contract explicit for whoever picks up the
next slice. I'd treat the `incompatible-changes.md` note as optional here,
since no incompatibility exists yet — worth adding once STIP-33 actually lands
the new field, at the PR that introduces it.
**Issue 4 (opaque Base64 diff on nested descriptors, Low)** — agreed this
would help debuggability if `TaskGroupLocation` or `ExecutionState` ever
drifts, but I don't think it's worth doing in this PR. It's real follow-up
value, not a gap in what this PR is claiming.
**Issue 5 (test package placement, Low)** — agreed as a nit; `execution` is
the more discoverable home given `TaskGroupLocation`/`ExecutionState` live
there and the next slice will be looking for this test next to the classes it's
touching. Non-blocking, author's call whether to fold it in now or as a
follow-up.
None of this changes my read on the production diff itself — two harmless
constants, verified against the pre-PR class shape, with the byte-for-byte
fixtures proving today's wire form is unchanged. My approval stands on that.
I'd just ask that Issues 1 and 3 land as part of this PR before merge, since
they're the two-line Javadoc additions that keep the next STIP-33 slice from
tripping over exactly the ambiguity you've flagged.
--
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]
