davidzollo commented on PR #10432: URL: https://github.com/apache/seatunnel/pull/10432#issuecomment-3840127302
CI failed, you can check the detailed info `https://github.com/AshharAhmadKhan/seatunnel/actions/runs/21617986762/job/62300847145` There is a significant type mismatch error in the code that will cause the build to fail. **File**: `seatunnel-connectors-v2/connector-socket/src/main/java/org/apache/seatunnel/connectors/seatunnel/socket/sink/SocketSink.java` ```java @Override public AbstractSinkWriter<SeaTunnelRow, Void> createWriter(SinkWriter.Context context) throws IOException { // [Error] SocketSinkWriter constructor requires SeaTunnelRowType, but CatalogTable was passed return new SocketSinkWriter(socketConfig, catalogTable); } ``` **File**: `seatunnel-connectors-v2/connector-socket/src/main/java/org/apache/seatunnel/connectors/seatunnel/socket/sink/SocketSinkWriter.java` ```java SocketSinkWriter(SocketConfig socketConfig, SeaTunnelRowType seaTunnelRowType) throws IOException { // ... } ``` **Suggestion**: Modify `SocketSink.java` to call `.getSeaTunnelRowType()`: ```java return new SocketSinkWriter(socketConfig, catalogTable.getSeaTunnelRowType()); ``` ###Insufficient Test Coverage Although `MultiTableSocketSinkTest.java` was added, it has serious flaws: 1. **Avoiding Core Paths**: The test code only instantiates `SocketSink` but explicitly comments that it **does not call** `createWriter`. ```java // Note: We don't actually create the writer here because it would try to open a socket... ``` 2. **Masking Compilation Errors**: Because `createWriter` is not actually called in the test, the compilation error mentioned above is hidden during the testing phase. 3. **Lack of Write Testing**: There is no verification that multi-table data can actually be sent through the Socket. **Suggestion**: Add integration tests or Mock tests. You must call `createWriter` and perform `write` operations to ensure the code logic is correct. You can use Mock objects to avoid real Socket connections or start a local ServerSocket for testing. For a correct implementation guide, please refer to **`ElasticsearchSink`**. - It correctly implements `SupportMultiTableSink`. - Its structure is similar to Socket (Network Client -> Serialization -> Send), making it a good reference for this PR. -- 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]
