[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388

Carl-Zhou-CN · 2023-08-28T08:12:12Z

Mainly to deal with databases like Oracle that cannot concatenate parameters in the URL.

Purpose of this pull request

Check list

Code changed are covered with tests, or it does not need tests for reason:
If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If you are contributing the connector code, please check that the following files are updated:
1. Update change log that in connector document. For more details you can refer to connector-v2
2. Update plugin-mapping.properties and add new connector information in it
3. Update the pom file of seatunnel-dist
Update the release-note.

zhilinli123 · 2023-09-08T08:59:50Z

...nector-jdbc-e2e/connector-jdbc-e2e-part-1/src/test/resources/jdbc_mysql_source_and_sink.conf

    driver = "com.mysql.cj.jdbc.Driver"
    connection_check_timeout_sec = 100
    user = "root"
    password = "Abc!@#135_seatunnel"

    query = "select * from source;"
+     properties {
+       useSSL=false
+       rewriteBatchedStatements=true


rewriteBatchedStatements=true Can you add this configuration to option by default? If the user does not set it manually, we can also enable this optimization in the demo

I have already added it. Please take a look and let me know if it meets the requirements.

This is I want too. Set by default rather than teach user to configure rewriteBatchedStatements is more reasonable.

EricJoy2048 · 2023-09-12T08:07:50Z

Please resolve conflicts.

EricJoy2048

LGTM, Please resolve the conflicts.

EricJoy2048 · 2023-09-12T08:13:23Z

LGTM, Please resolve the conflicts.

I am sorry, document need be update.

Carl-Zhou-CN · 2023-09-13T10:27:57Z

LGTM, Please resolve the conflicts.

I am sorry, document need be update.

@EricJoy2048 done

zhilinli123 · 2023-09-18T03:26:45Z

Can you provide screenshots for this? Make sure he's valid
@Carl-Zhou-CN
CC @hailin0 @ic4y

EricJoy2048

LGTM, A little problem.

EricJoy2048 · 2023-09-18T09:48:30Z

docs/en/connector-v2/sink/DB2.md

@@ -72,6 +72,7 @@ semantics (using XA transaction guarantee).
 | max_commit_attempts                       | Int     | No       | 3       | The number of retries for transaction commit failures                                                                                                                                                                                        |
 | transaction_timeout_sec                   | Int     | No       | -1      | The timeout after the transaction is opened, the default is -1 (never timeout). Note that setting the timeout may affect<br/>exactly-once semantics                                                                                          |
 | auto_commit                               | Boolean | No       | true    | Automatic transaction commit is enabled by default                                                                                                                                                                                           |
+| properties                                |         | No       | -       | Additional connection configuration parameters                                                                                                                                                                                               |


The type of properties is map ?

EricJoy2048 · 2023-09-18T09:48:54Z

docs/en/connector-v2/sink/Jdbc.md

@@ -48,6 +48,7 @@ support `Xa transactions`. You can set `is_exactly_once=true` to enable it.
 | transaction_timeout_sec                   | Int     | No       | -1            |
 | auto_commit                               | Boolean | No       | true          |
 | field_ide                                 | String  | No       | -             |
+| properties                                |         | No       | -             |


Same as above.

Carl-Zhou-CN · 2023-09-18T10:44:32Z

Can you provide screenshots for this? Make sure he's valid @Carl-Zhou-CN CC @hailin0 @ic4y

EricJoy2048

LGTM

Hisoka-X

Over all looking great. But could you add some UT to cover some case?

User configure parameter in both url and properties with different key.
User configure parameter in both url and properties with same key different value (also should metion it in the doc which value will be choose, I think the level of url should be higher).
Only properties or only in url.
With these case, you should use UT to check the parameter result is you want.

Carl-Zhou-CN · 2023-10-12T14:24:21Z

Over all looking great. But could you add some UT to cover some case?

User configure parameter in both url and properties with different key.

User configure parameter in both url and properties with same key different value (also should metion it in the doc which value will be choose, I think the level of url should be higher).

Only properties or only in url.
With these case, you should use UT to check the parameter result is you want.

done

Hisoka-X · 2023-10-13T11:04:26Z

...rc/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/config/JdbcConnectionConfig.java

@@ -133,6 +142,7 @@ public static final class Builder {
        private String xaDataSourceClassName;
        private int maxCommitAttempts = JdbcOptions.MAX_COMMIT_ATTEMPTS.defaultValue();
        private int transactionTimeoutSec = JdbcOptions.TRANSACTION_TIMEOUT_SEC.defaultValue();
+        private Map<String, String> info;


Why not call it properties?

Suggested change

private Map<String, String> info;

private Map<String, String> properties;

Hisoka-X · 2023-10-13T11:12:55Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+
+    void defaultSinkParametersTest() throws IOException, SQLException, ClassNotFoundException {
+        // case1 url not contains parameters and properties not contains parameters
+        JdbcSink jdbcSink1 = new JdbcSink();


Please use JdbcSinkFactory to create JdbcSink. We will remove prepare method in the future.

Hisoka-X · 2023-10-13T11:13:01Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+        Assertions.assertEquals(connectionProperties1.get("rewriteBatchedStatements"), "true");
+
+        // case2 url contains parameters and properties not contains parameters
+        JdbcSink jdbcSink2 = new JdbcSink();


Hisoka-X · 2023-10-13T11:15:11Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+
+    void defaultSourceParametersTest() throws IOException, SQLException, ClassNotFoundException {
+        // case1 url not contains parameters and properties not contains parameters
+        JdbcSource jdbcSource1 = new JdbcSource();


Hisoka-X · 2023-10-13T11:16:46Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+    }
+
+    @NotNull private HashMap<String, Object> getMap() {
+        HashMap<String, Object> map = new HashMap<>();


Suggested change

HashMap<String, Object> map = new HashMap<>();

Map<String, Object> map = new HashMap<>();

Hisoka-X · 2023-10-13T11:17:11Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+        Assertions.assertEquals(connectionProperties4.get("rewriteBatchedStatements"), "false");
+    }
+
+    @NotNull private HashMap<String, Object> getMap() {


Suggested change

@NotNull private HashMap<String, Object> getMap() {

@NotNull private Map<String, Object> getDefaultConfigMap() {

Hisoka-X · 2023-10-13T11:18:03Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+
+        // case3 url not contains parameters and properties not contains parameters
+        JdbcSource jdbcSource3 = new JdbcSource();
+        HashMap<String, Object> map3 = getMap();


Please use Map not HashMap to keep code style.

Hisoka-X · 2023-10-13T11:19:52Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+        // case2 url contains parameters and properties not contains parameters
+        JdbcSource jdbcSource2 = new JdbcSource();
+        HashMap<String, Object> map2 = getMap();
+        map2.put("url", getUrl() + "?rewriteBatchedStatements=false");
+        map2.put("query", SQL);
+        Config config2 = ConfigFactory.parseMap(map2);
+        Properties connectionProperties2 = getSourceProperties(jdbcSource2, config2);
+        Assertions.assertEquals(connectionProperties2.get("rewriteBatchedStatements"), "true");


Why we change rewriteBatchedStatements to false in url but get true in connectionProperties?

Why we change rewriteBatchedStatements to false in url but get true in connectionProperties?

I think this test case means the param set in Properties will overwrite param set in jdbc url.

But it overwrited by default value. I think the priority should be: properties map > url > default properties. Not : properties map > default properties > url.

yes, you're right

Hisoka-X · 2023-10-13T11:22:16Z

...dbc-e2e-part-1/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlIT.java

+    private static Object getFieldValue(Object object, String name) {
+        Class objClass = object.getClass();
+        Field[] fields = objClass.getDeclaredFields();
+        for (Field field : fields) {
+            try {
+                String fieldName = field.getName();
+                if (fieldName.equalsIgnoreCase(name)) {
+                    field.setAccessible(true);
+                    return field.get(object);
+                }
+            } catch (SecurityException e) {
+
+            } catch (IllegalAccessException e) {
+                throw new RuntimeException(e);
+            }
+        }
+        return null;
+    }


Please use ReflectionUtils::getField to replace it.

Hisoka-X

LGTM if test passed.

EricJoy2048

LGTM

Hisoka-X · 2023-10-18T03:10:18Z

Thanks @Carl-Zhou-CN and all!

zhilinli123 reviewed Sep 8, 2023

View reviewed changes

EricJoy2048 reviewed Sep 12, 2023

View reviewed changes

zhouyao added 3 commits September 13, 2023 17:18

[Feature][Jdbc] Supporting more ways to configure connection parameters.

e4511f8

[Feature][JDBC] Default parameter

9a4c6f9

[Feature][Jdbc] Supporting more ways to configure connection parameters

d6fae86

Carl-Zhou-CN force-pushed the jdbc-p branch from fcfbe70 to d6fae86 Compare September 13, 2023 10:24

[Feature][Jdbc] Supporting more ways to configure connection parameters

46da741

EricJoy2048 added this to the 2.3.4 milestone Sep 15, 2023

Merge branch 'apache:dev' into jdbc-p

bef925c

EricJoy2048 reviewed Sep 18, 2023

View reviewed changes

[Feature][Jdbc] Supporting more ways to configure connection parameters

4aea653

EricJoy2048 previously approved these changes Sep 18, 2023

View reviewed changes

github-actions bot added approved reviewed labels Sep 18, 2023

zhilinli123 approved these changes Sep 19, 2023

View reviewed changes

Hisoka-X reviewed Sep 19, 2023

View reviewed changes

[Feature][jdbc] UT

ad81a87

Carl-Zhou-CN dismissed EricJoy2048’s stale review via ad81a87 September 21, 2023 09:08

github-actions bot removed the approved label Sep 21, 2023

zhouyao and others added 6 commits September 21, 2023 17:14

[Feature][jdbc] UT

478c385

[Feature][jdbc] UT

7046537

[Feature][jdbc] UT

98eb00c

[Feature][jdbc] UT

38d164f

Merge branch 'apache:dev' into jdbc-p

c887084

[Feature][jdbc] UT

d7e4703

zhouyao and others added 14 commits September 24, 2023 22:02

[Feature][jdbc] UT

5dbe8b8

[Feature][jdbc] UT

16ef200

[Feature][cdc] UT

6f2a781

[Feature][cdc] UT

4c78b1b

[Feature][cdc] UT

7d606da

[Feature][cdc] UT

bc6a98a

[Feature][cdc] UT

9169f2d

[Feature][cdc] UT

886a39a

[Feature][cdc] UT

0b0d9e1

Merge branch 'apache:dev' into jdbc-p

e568fdf

[Improve][test] UT

e368b60

Merge branch 'apache:dev' into jdbc-p

de876f0

UT

304c3de

UT

0968ef4

Carl-Zhou-CN requested review from Hisoka-X and EricJoy2048 October 12, 2023 14:23

Hisoka-X reviewed Oct 13, 2023

View reviewed changes

zhouyao added 4 commits October 16, 2023 19:56

fix

6b52905

fix

ac1211d

fix

53488a9

fix

2e69042

Hisoka-X approved these changes Oct 17, 2023

View reviewed changes

github-actions bot added the approved label Oct 17, 2023

EricJoy2048 approved these changes Oct 18, 2023

View reviewed changes

EricJoy2048 merged commit d31e947 into apache:dev Oct 18, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388

[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388

Carl-Zhou-CN commented Aug 28, 2023

zhilinli123 Sep 8, 2023

Carl-Zhou-CN Sep 11, 2023

Hisoka-X Sep 19, 2023

EricJoy2048 commented Sep 12, 2023

EricJoy2048 left a comment

EricJoy2048 commented Sep 12, 2023

Carl-Zhou-CN commented Sep 13, 2023

zhilinli123 commented Sep 18, 2023 •

edited

Loading

EricJoy2048 left a comment

EricJoy2048 Sep 18, 2023

EricJoy2048 Sep 18, 2023

Carl-Zhou-CN commented Sep 18, 2023

EricJoy2048 left a comment

Hisoka-X left a comment

Carl-Zhou-CN commented Oct 12, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Hisoka-X Oct 13, 2023

Carl-Zhou-CN Oct 16, 2023

Hisoka-X Oct 13, 2023

EricJoy2048 Oct 13, 2023

Hisoka-X Oct 13, 2023

Carl-Zhou-CN Oct 16, 2023

Hisoka-X Oct 13, 2023

Carl-Zhou-CN Oct 16, 2023

Hisoka-X left a comment

EricJoy2048 left a comment

Hisoka-X commented Oct 18, 2023

	private Map<String, String> info;
	private Map<String, String> properties;

	HashMap<String, Object> map = new HashMap<>();
	Map<String, Object> map = new HashMap<>();

	@NotNull private HashMap<String, Object> getMap() {
	@NotNull private Map<String, Object> getDefaultConfigMap() {

[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388

[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388

Conversation

Carl-Zhou-CN commented Aug 28, 2023

Purpose of this pull request

Check list

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricJoy2048 commented Sep 12, 2023

EricJoy2048 left a comment

Choose a reason for hiding this comment

EricJoy2048 commented Sep 12, 2023

Carl-Zhou-CN commented Sep 13, 2023

zhilinli123 commented Sep 18, 2023 • edited Loading

EricJoy2048 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carl-Zhou-CN commented Sep 18, 2023

EricJoy2048 left a comment

Choose a reason for hiding this comment

Hisoka-X left a comment

Choose a reason for hiding this comment

Carl-Zhou-CN commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hisoka-X left a comment

Choose a reason for hiding this comment

EricJoy2048 left a comment

Choose a reason for hiding this comment

Hisoka-X commented Oct 18, 2023

zhilinli123 commented Sep 18, 2023 •

edited

Loading