amogh-jahagirdar commented on code in PR #10140:
URL: https://github.com/apache/iceberg/pull/10140#discussion_r1588457797
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -40,6 +40,8 @@ final class JdbcUtil {
// property to control if view support is added to the existing database
static final String SCHEMA_VERSION_PROPERTY = JdbcCatalog.PROPERTY_PREFIX +
"schema-version";
+ static final String RETRYABLE_STATUS_CODES = "retryable_status_codes";
Review Comment:
> that anything specific to JDBC catalog should be prefixed by jdbc, just
to be sure we don't have conflict with properties with same name not related to
JDBC Catalog. As JDBC Connection is used in JDBC Catalog, I would prefix with
jdbc for consistency. As it's user facing properties, it's important to be
consistent here.
I think if we do this it's not really consistent with what we do with other
catalogs. For example, REST, Glue, Nessie catalogs don't neccessarily prefix
any of their custom configurations with "glue", "rest", or "nessie"
respectively; configuring
the catalog configuration is just a matter of setting the property via
`<catalog_name>.<property>`. I also don't think we need to worry about
conflicts in property names across different catalogs; in the end we use
reflection to load the catalog and it's up to the catalog implementation to
respect whichever properties we want, so we just need to make sure the
properties are well defined.
The docs seemed pretty clear to me about the original intention of the
"jdbc" prefix for the JDBC catalog
https://iceberg.apache.org/docs/nightly/jdbc/?h=jdbc#configurations is that
it's specifically meant for configuring JDBC connections (username/port/db name
that are in the URL itself).
`retryable_status_codes` is *not* configuring the JDBC connection URL.
It is a catalog level property which controls when the catalog should retry
establishing an entirely new connection. That's why I think it should be
namespaced as it's own property (same as any other property that we do for
other catalogs).
I feel like that's actually more consistent behavior for a user; they can
know that everything under jdbc is part of the actual connection URL, and
everything outside of the JDBC prefix is a general catalog level property.
--
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]
