RocMarshal commented on code in PR #183:
URL: 
https://github.com/apache/flink-connector-jdbc/pull/183#discussion_r2685085862


##########
flink-connector-jdbc-core/src/test/java/org/apache/flink/connector/jdbc/core/database/catalog/AbstractJdbcCatalogTest.java:
##########
@@ -27,16 +27,33 @@ class AbstractJdbcCatalogTest {
 
     @Test
     void testJdbcUrl() {
-        AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234/");
-        AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234");
+        
AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234/db", "db");
+        
AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234/db", null);
+        AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234/", 
"db");
+        AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234", 
"db");

Review Comment:
   `AbstractJdbcCatalog.validateJdbcUrl("jdbc:dialect://localhost:1234", null);`
   
   What would happen if it were used in this way?
   
   Or, put more plainly: when a user’s input does not meet the requirements, it 
might be much better if we could guide the user by providing richer exception 
messages.
   
   For example:
   
   - Why did the error occur?
   - How can this error be avoided, and what is the correct way to configure it?
   - etc.
   
   With the current exception messages, they may be somewhat obscure.
   For instance, an error like the following is reported:
   ```
   java.lang.IllegalArgumentException
       at 
org.apache.flink.util.Preconditions.checkArgument(Preconditions.java:xxxx)
       at 
org.apache.flink.connector.jdbc.core.database.catalog.AbstractJdbcCatalog.validateJ
   ```



-- 
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]

Reply via email to