DanielLeens commented on PR #12416: URL: https://github.com/apache/seatunnel/pull/12416#issuecomment-5770886399
Thanks for the deeper pass, @SEZ9 — the head is still `e6ee622` (unchanged since my approval), so I'll respond in place rather than re-review. **Issue 1 (sourceConfig round trip proves non-null, not value-fidelity) — same gap I flagged as my own Issue 1, still Low for me, not Medium.** The reason I don't raise it to Medium: `MqttSourceConfig`'s field-level serialization is already exhaustively proven in isolation by `MqttSourceConfigTest.testConfigSurvivesJavaSerialization` (all 13 getters, non-zero/non-default values chosen specifically so a dropped field can't hide behind a default). Nesting that same object inside `MqttSource` doesn't introduce a second, independent serialization path — Java serialization walks the object graph field-by-field regardless of which root object triggered it, so a `transient` (or otherwise dropped) field inside `MqttSourceConfig` fails `MqttSourceConfigTest` the same way whether or not `MqttSourceTest` also drives a reader. What `MqttSourceTest.testSourceIsJavaSerializable` adds on top is narrower: it only needs to prove `MqttSource`'s own reference to `sourceConfig` isn't itself dropp ed, which the constructor's `getMaxQueueSize()`/`getFormat()` dereference already NPEs on. Driving the restored reader end-to-end (your suggested `messageArrived` probe, which is also the improvement I proposed) is good defense-in-depth against a regression class I can't currently construct, so I'd keep this as an optional follow-up rather than a blocker. Happy to be shown a concrete scenario where the two tests' guarantees actually diverge — if one exists, I'll revise. **Issues 2 & 4 (dropped STREAMING `UNBOUNDED` assertion) — you're right, and this is sharper than my own reasoning in the approval.** I focused on the fact that the STREAMING check can't distinguish "context restored" from "context lost" (both return `UNBOUNDED`), and concluded it therefore had no discriminating power. I didn't credit the case you point out: a `jobContext` that survives with a corrupted/null `jobMode` would throw in the STREAMING case too (since `STREAMING.equals(null)` is `false`), so the removed assertion was catching that partial-corruption case even though it couldn't catch total loss. That's a real, narrow gap the new BATCH-only test doesn't cover. I'd support restoring `assertEquals(Boundedness.UNBOUNDED, restored.getBoundedness())` in `testSourceIsJavaSerializable` as the happy-path complement, one line, non-blocking. **Issues 3 & 5 (comment wording nit; shared `fullConfig()` helper to stop the two option lists drifting)** — agreed, both Low, non-blocking polish. Net: nothing here changes my merge recommendation. All open items are Low (optional coverage-depth / wording / maintainability nits), including the one I'd call out as most worth a quick fold-in (restoring the STREAMING `UNBOUNDED` line, since it's a one-liner and directly responds to Issue 2/4). My **APPROVED** review stands — no blocking issues on the current head. @SEPURI-SAI-KRISHNA, up to you whether to fold in the STREAMING assertion and/or the `messageArrived` probe as a quick follow-up commit; either way this doesn't need to hold up merge from my side. -- 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]
