-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature][Jdbc] Supporting more ways to configure connection parameters. #5388
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already added it. Please take a look and let me know if it meets the requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is I want too. Set by default rather than teach user to configure rewriteBatchedStatements
is more reasonable.
Please resolve conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Please resolve the conflicts.
I am sorry, document need be update. |
fcfbe70
to
d6fae86
Compare
@EricJoy2048 done |
Can you provide screenshots for this? Make sure he's valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, A little problem.
docs/en/connector-v2/sink/DB2.md
Outdated
@@ -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 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of properties is map
?
docs/en/connector-v2/sink/Jdbc.md
Outdated
@@ -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 | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call it properties
?
private Map<String, String> info; | |
private Map<String, String> properties; |
|
||
void defaultSinkParametersTest() throws IOException, SQLException, ClassNotFoundException { | ||
// case1 url not contains parameters and properties not contains parameters | ||
JdbcSink jdbcSink1 = new JdbcSink(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use JdbcSinkFactory
to create JdbcSink
. We will remove prepare
method in the future.
Assertions.assertEquals(connectionProperties1.get("rewriteBatchedStatements"), "true"); | ||
|
||
// case2 url contains parameters and properties not contains parameters | ||
JdbcSink jdbcSink2 = new JdbcSink(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
void defaultSourceParametersTest() throws IOException, SQLException, ClassNotFoundException { | ||
// case1 url not contains parameters and properties not contains parameters | ||
JdbcSource jdbcSource1 = new JdbcSource(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
@NotNull private HashMap<String, Object> getMap() { | ||
HashMap<String, Object> map = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap<String, Object> map = new HashMap<>(); | |
Map<String, Object> map = new HashMap<>(); |
Assertions.assertEquals(connectionProperties4.get("rewriteBatchedStatements"), "false"); | ||
} | ||
|
||
@NotNull private HashMap<String, Object> getMap() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NotNull private HashMap<String, Object> getMap() { | |
@NotNull private Map<String, Object> getDefaultConfigMap() { |
|
||
// case3 url not contains parameters and properties not contains parameters | ||
JdbcSource jdbcSource3 = new JdbcSource(); | ||
HashMap<String, Object> map3 = getMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Map
not HashMap
to keep code style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we change rewriteBatchedStatements
to false
in url but get true
in connectionProperties
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we change
rewriteBatchedStatements
tofalse
in url but gettrue
inconnectionProperties
?
I think this test case means the param set in Properties will overwrite param set in jdbc url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it overwrited by default value. I think the priority should be: properties map > url > default properties
. Not : properties map > default properties > url
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you're right
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ReflectionUtils::getField
to replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if test passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @Carl-Zhou-CN and all! |
Mainly to deal with databases like Oracle that cannot concatenate parameters in the URL.
Purpose of this pull request
Check list
New License Guide
release-note
.