SEPURI-SAI-KRISHNA commented on PR #12416: URL: https://github.com/apache/seatunnel/pull/12416#issuecomment-5771544424
Thanks @SEZ9 and @DanielLeens. Issues 2, 3 and 4 are addressed in the push above, and I took Issue 1 as well. Issue 5 I have not done, with reasoning below. **Issues 2 and 4.** Agreed, and the case I missed is the one @SEZ9 names: a context restored with a null `jobMode` throws in the STREAMING path too, because `STREAMING.equals(null)` is false, so the BATCH test cannot tell it apart from a genuine BATCH context. `assertEquals(Boundedness.UNBOUNDED, restored.getBoundedness())` is back in `testSourceIsJavaSerializable`, and the comment in `testJobContextSurvivesJavaSerialization` now says the two are complementary rather than that the STREAMING check is uninformative. My original reasoning was right that it cannot detect total loss and wrong to conclude it therefore had no discriminating power. **Issue 3.** Correct, `sourceConfig` is the first declared field, not the third. I dropped the ordinal entirely rather than fixing the number, as you suggested, so it cannot go stale if the fields are reordered. **Issue 1.** Implemented as you described. The source now sets `max_queue_size = 1`, which is not the default, and the restored reader is driven past that limit, mirroring `testReaderFailsWhenMessageQueueIsFull`. @DanielLeens, on your question about whether the two tests' guarantees actually diverge: for a pure serialization defect I think you are right and I could not construct a counterexample. Java serialization walks the graph field by field, so a `transient` field inside `MqttSourceConfig` fails `MqttSourceConfigTest` whether or not `MqttSourceTest` drives a reader. I checked that rather than assuming it, by marking `maxQueueSize` transient: both the old and the new assertion fail, because `new LinkedBlockingQueue<>(0)` throws `IllegalArgumentException` at construction. So that mutation does not separate them. Where they do separate is a different class of defect, config to reader wiring rather than serialization. Hardcoding the capacity in `MqttSourceReader`: ```java this.messageQueue = new LinkedBlockingQueue<>(2048); // was sourceConfig.getMaxQueueSize() ``` leaves `createReader` succeeding, so the old `assertDoesNotThrow` passes, while the new assertion fails with "Expected MqttConnectorException to be thrown, but nothing was thrown". I should be precise about what that proves: `testReaderFailsWhenMessageQueueIsFull` also catches it, so the mutation is not uniquely caught by the new code. What is new is that it is now caught on a *deserialized* source, so a defect that only manifests after a round trip has a guard it did not have before. That is narrower than Medium in my view, closer to your Low, but the assertion is one line and it replaced one that asserted nothing, so it seemed worth folding in either way. **Issue 5.** Not done. A shared `fullConfig()` helper is the right instinct and the drift you describe is real, but it means rewriting `testExplicitValues`, which is not part of this PR and is not broken. I would rather not widen a test-coverage PR into a refactor of a neighbouring test. Happy to do it as a separate change if you would like it. Verification: 34 tests pass in `connector-mqtt`, `spotless:check` is green, and the three main classes are byte-identical to `dev`. -- 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]
