kbendick commented on a change in pull request #3078:
URL: https://github.com/apache/iceberg/pull/3078#discussion_r704703334
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcClientPool.java
##########
@@ -48,7 +48,12 @@
protected Connection newClient() {
try {
Properties dbProps = new Properties();
- properties.forEach((key, value) ->
dbProps.put(key.replace(JdbcCatalog.PROPERTY_PREFIX, ""), value));
+ properties.forEach((key, value) -> {
+ if (key.startsWith(JdbcCatalog.PROPERTY_PREFIX)) {
+ String dbKey = key.replaceFirst("^" + JdbcCatalog.PROPERTY_PREFIX,
"");
Review comment:
Also, could this be simplified to not use the regex? If not that's ok,
just generally not a fan of regex if possible. I looked at some of the
surrounding class and it does seem they use a regex, so maybe it's the string
concatenation that's getting to me?
Also, do we need to strip more than just `jdbc` from the string at this
point? And do we need to check that these properties are specifically for this
JDBC catalog?
For example, if I were using spark-sql and I had two catalogs,
`spark.sql.catalog.jdbc_1` and `spark.sql.catalog.jdbc_2`, should we do a name
check here or is that already handled? And is there any chance the regex will
do something we don't intend for it to do if the catalog is named `jdbc_foo`?
I'm guessing not because of `replaceFirst`.
Either way, this looks much better to me.
**TLDR** - Can you add test cases around configuring the right jdbc user as
well as a test case with two jdbc catalogs with two different jdbc users-
possibly in `TestJdbcCatalog` (or say, from Spark or from Flink if it's hard to
test from there for some reason)?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
