och5351 commented on code in PR #183:
URL:
https://github.com/apache/flink-connector-jdbc/pull/183#discussion_r2601331389
##########
flink-connector-jdbc-core/src/main/java/org/apache/flink/connector/jdbc/JdbcConnectionOptions.java:
##########
@@ -34,6 +34,7 @@ public class JdbcConnectionOptions implements Serializable {
public static final String USER_KEY = "user";
public static final String PASSWORD_KEY = "password";
+ public static final String DATABASE_OPTIONS = "database-options";
Review Comment:
Hi @RocMarshal !
Thank you for your helpful review.
I agree with your point, but the current version does not support applying
DB parameters in the catalog configuration (although it’s possible in dynamic
tables).
Here is the current code snippet:
```java
public AbstractJdbcCatalog(
ClassLoader userClassLoader,
String catalogName,
String defaultDatabase,
String baseUrl,
Properties connectionProperties) {
super(catalogName, defaultDatabase);
checkNotNull(userClassLoader);
checkArgument(!StringUtils.isNullOrWhitespaceOnly(baseUrl));
validateJdbcUrl(baseUrl);
this.userClassLoader = userClassLoader;
this.baseUrl = baseUrl.endsWith("/") ? baseUrl : baseUrl + "/";
this.defaultUrl = getDatabaseUrl(defaultDatabase);
this.connectionProperties =
Preconditions.checkNotNull(connectionProperties);
checkArgument(
!StringUtils.isNullOrWhitespaceOnly(connectionProperties.getProperty(USER_KEY)));
checkArgument(
!StringUtils.isNullOrWhitespaceOnly(
connectionProperties.getProperty(PASSWORD_KEY)));
}
protected String getDatabaseUrl(String databaseName) {
return baseUrl + databaseName;
}
```
If the base-url is given like this:
```sql
CREATE CATALOG my_mariadb_catalog WITH (
'type' = 'jdbc',
'base-url' =
'jdbc:mysql://localhost:3306/my_db?serverTimezone=UTC&tinyInt1isBit=false',
'username' = 'user',
'password' = 'pass',
'default-database' = 'my_db',
-- 'database-options' = 'serverTimezone=UTC&tinyInt1isBit=false'
);
```
then the Flink JDBC connector treats the URL as:
```
jdbc:mysql://localhost:3306/my_db?serverTimezone=UTC&tinyInt1isBit=false/my_db
```
Because of this, I initially proposed the use of database-options.
However, I think it would also be good to make better use of base-url
directly.
Therefore, I would like to suggest the following possible improvements:
## Option 1:
1. Change `default-database` in `JdbcCatalogFactory.java` to be an optional
parameter.
2. Remove the logic in the AbstractJdbcCatalog constructor that appends `/`
to base-url.
3. Modify the getDatabaseUrl() method in AbstractJdbcCatalog to work as
follows:
* Check if defaultDatabase is provided.
* Check if the baseUrl contains ? indicating database options.
* Extract the database name from baseUrl (e.g., using split("?")[0] and
checking if it ends with /).
* Return the appropriate baseUrl.
## Option 2:
1. Remove the default-database option from JdbcCatalogFactory.java
altogether.
1. Accept the entire connection URL (including database and options) in the
base-url parameter.
Thank you again for your valuable review and continued support.
I would appreciate your thoughts on this proposal and am happy to discuss
further.
Best regards.
--
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]
