Skip to content

Conversation

@jia17
Copy link
Contributor

@jia17 jia17 commented Apr 22, 2025

Purpose of this pull request

[Feature][TDengine] Support multi-table sink feature #5652

Does this PR introduce any user-facing change?

How was this patch tested?

a new e2e test

Check list

jia zhang added 30 commits April 22, 2025 13:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for multi‐table sink functionality for the TDengine connector, enabling tests to write to multiple tables. Key changes include:

  • Removing TDengine from the exclusion list in integration tests.
  • Introducing new multi‐table sink test cases and constants in TDengineIT.
  • Refactoring configuration and sink writer construction to support multi‐table operations.

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
seatunnel-e2e/seatunnel-core-e2e/seatunnel-starter-e2e/src/test/java/org/apache/seatunnel/core/starter/seatunnel/SeaTunnelConnectorTest.java Removed TDengine exclusion to enable TDengine tests.
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java Added multi-table sink test cases, constants, and updated query methods.
seatunnel-connectors-v2/connector-tdengine/src/test/java/org/apache/seatunnel/connectors/seatunnel/tdengine/sink/TDengineSinkWriterTest.java Refactored sink writer tests to use the new builder-based configuration.
seatunnel-connectors-v2/connector-tdengine/src/main/java/org/apache/seatunnel/connectors/seatunnel/tdengine/source/TDengineSourceFactory.java Updated source factory to support TDengine.
seatunnel-connectors-v2/connector-tdengine/src/main/java/org/apache/seatunnel/connectors/seatunnel/tdengine/sink/*.java Modified sink writer, sink, and sink factory classes to implement multi-table support.
seatunnel-connectors-v2/connector-tdengine/src/main/java/org/apache/seatunnel/connectors/seatunnel/tdengine/config/* Added and updated source and sink configuration and options.
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ConnectorOptionCheckTest.java Updated connector option whitelist by removing TDengine options.
Files not reviewed (1)
  • seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/resources/tdengine/tdengine_fake_to_sink_multitable.conf: Language not supported
Comments suppressed due to low confidence (2)

seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java:62

  • [nitpick] Variable name 'testDataCountMulti_Table1' uses underscores which is unconventional in Java non-constant variable naming. Consider renaming it to 'testDataCountMultiTable1' to follow camelCase conventions.
private final int testDataCountMulti_Table1 = 5;

seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java:63

  • [nitpick] Variable name 'testDataCountMulti_Table2' uses underscores which is unconventional in Java non-constant variable naming. Consider renaming it to 'testDataCountMultiTable2' to improve readability.
private final int testDataCountMulti_Table2 = 7;

Comment on lines 36 to 46
public static final Option<String> USERNAME =
Options.key("username")
.stringType()
.noDefaultValue()
.withDescription("The username for TDengine authentication");

public static final Option<String> PASSWORD =
Options.key("password")
.stringType()
.noDefaultValue()
.withDescription("The password for TDengine authentication");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all common options in source and sink into abstract common options class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've refactored the code to move repeated options into a common options.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for a multi‐table sink feature for TDengine by updating both the integration tests and the TDengine connector code. Key changes include removing TDengine from the exclusion list in integration tests, adding new multi-write test cases and table creation for additional targets, and refactoring connector configurations and sink writer implementations to support multi‐table writing.

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.

File Description
seatunnel-e2e/seatunnel-core-e2e/seatunnel-starter-e2e/src/test/java/org/apache/seatunnel/core/starter/seatunnel/SeaTunnelConnectorTest.java Removed exclusion for TDengine to include its test runs.
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java Added new tests and table creation commands for multi-table sink support.
seatunnel-connectors-v2/connector-tdengine/* Updated source and sink factories, configuration classes, and sink writer to support multi-table features.
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ConnectorOptionCheckTest.java Removed TDengine options from the whitelist to align with the updated connector tests.
Files not reviewed (1)
  • seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/resources/tdengine/tdengine_fake_to_sink_multitable.conf: Language not supported
Comments suppressed due to low confidence (2)

seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java:62

  • [nitpick] Consider renaming 'testDataCountMulti_Table1' to 'testDataCountMultiTable1' to conform with standard Java camelCase naming conventions.
private final int testDataCountMulti_Table1 = 5;

seatunnel-e2e/seatunnel-connector-v2-e2e/connector-tdengine-e2e/src/test/java/org/apache/seatunnel/e2e/connector/tdengine/TDengineIT.java:63

  • [nitpick] Consider renaming 'testDataCountMulti_Table2' to 'testDataCountMultiTable2' to maintain consistency with Java naming conventions.
private final int testDataCountMulti_Table2 = 7;

@hailin0 hailin0 merged commit 98b593f into apache:dev Apr 23, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants