DanielLeens commented on PR #11557: URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5999229016
@SEZ9 Thanks, all three F1 asks are addressed in `5a0f93b37de`. **1. Explicit wire-type check.** `ManagedSourceRegisterOperation#readInternal` no longer uses the typed override. It now decodes the reader location by the wire `factoryId`/`classId` (`in.readObject()`) and rejects anything that is not a `TaskLocation` (including `null`) with an explicit `IOException` at the read site, naming the expected and the decoded type. I chose this over comparing raw header ints because it relies only on the public `ObjectDataInput` contract instead of Hazelcast's internal `IdentifiedDataSerializable` header layout, and it also rejects non-`IdentifiedDataSerializable` payloads. The only classes the wire can select are the ones registered in the serializer factories, which is the same surface every Hazelcast operation already has; nothing decoded is used before the check. **2. Unit test.** Added `ManagedSourceRegisterOperationSerializationTest`: - `testMismatchedReaderLocationTypeIsRejected` writes the operation's exact field layout but puts a `Progress` (same serializer factory id as `TaskLocation`, different class id) in the reader-location slot and asserts the exact type-mismatch `IOException`. With the previous `readObject(TaskLocation.class)` this payload was read positionally into a `TaskLocation` instead of being rejected, so the test fails against the old code. - `testReaderLocationPayloadRoundTrips` asserts a genuine payload still decodes and re-encodes byte-for-byte. I have not run these locally; CI on the new head is the verification. **3. Other `readObject()` calls in the managed lane operations.** I checked every operation this PR adds or touches under `task/operation/source`: - `ManagedSourceCommandOperation` and `ManagedCoordinatorCommandOperation`: no `readObject()` at all, only primitives, strings and bounded byte arrays with explicit header validation. - `ManagedSourceRegisterOperation`: the one call fixed here. - `AssignSplitOperation#readInternal` has `taskID = in.readObject()` (line 107). That line is pre-existing on `dev`; this PR only adds a managed-lane branch to its run path. It is untyped and unchecked, so I would rather track it separately than widen this PR to the legacy operations, unless you prefer it fixed here. - The base `TaskOperation#readInternal` still uses `readObject(TaskLocation.class)` for its own `taskLocation` (also pre-existing on `dev`, shared by all task operations), so it has the same override-not-validate behavior described above. Same proposal: track separately. Separately, for transparency: `engine-v2-it` on this PR is still timing out inside the PR's own `ManagedSourceRuntimeIT` (managed-lane jobs stall right after reader open), so the Build will stay red until that is root-caused. It is unrelated to F1. -- 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]
