dimas-b commented on code in PR #3480:
URL: https://github.com/apache/polaris/pull/3480#discussion_r2748703026
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/ExternalCatalogFactory.java:
##########
@@ -33,24 +35,93 @@ public interface ExternalCatalogFactory {
/**
* Creates a catalog handle for the given connection configuration.
*
+ * @deprecated Implement {@link #createCatalog(ConnectionConfigInfoDpo,
PolarisCredentialManager,
+ * Map)} instead to support catalog properties pass-through (e.g., proxy
settings).
* @param connectionConfig the connection configuration
* @param polarisCredentialManager the credential manager for generating
connection credentials
* that Polaris uses to access external systems
* @return the initialized catalog
* @throws IllegalStateException if the connection configuration is invalid
*/
+ @Deprecated
Catalog createCatalog(
ConnectionConfigInfoDpo connectionConfig, PolarisCredentialManager
polarisCredentialManager);
+ /**
+ * Creates a catalog handle for the given connection configuration.
+ *
+ * <p>Implementations should override this method to receive catalog
properties (e.g.,
+ * rest.client.proxy.*, timeout settings). The default implementation
delegates to the deprecated
+ * 2-parameter version and logs a warning if properties are being ignored.
+ *
+ * @param connectionConfig the connection configuration
+ * @param polarisCredentialManager the credential manager for generating
connection credentials
+ * that Polaris uses to access external systems
+ * @param catalogProperties additional properties from the ExternalCatalog
entity that should be
+ * passed through to the underlying catalog (e.g., rest.client.proxy.*,
timeout settings).
+ * These are merged with lower precedence than connection config
properties.
+ * @return the initialized catalog
+ * @throws IllegalStateException if the connection configuration is invalid
+ */
+ default Catalog createCatalog(
+ ConnectionConfigInfoDpo connectionConfig,
+ PolarisCredentialManager polarisCredentialManager,
+ Map<String, String> catalogProperties) {
+ if (catalogProperties != null && !catalogProperties.isEmpty()) {
+ LoggerFactory.getLogger(getClass())
+ .warn(
+ "catalogProperties were provided but {} does not override
createCatalog with "
Review Comment:
This is a reasonable approach to ensuring backward compatibility with older
implementations of this interface.
However, Polaris core will always call this method (if I'm not mistaken),
and if a custom implementation does not override it, the server will be
producing a lot of WARN messages in runtime.
At the same time, in a new implementation of this interface it may be
tempting to the author to override only the old method (especially if helping
tools are involved), which would be sub-optimal since the intention is
obviously to override this method.
Our standing [evolution
guidelines](https://polaris.apache.org/in-dev/unreleased/evolution/) make it
clear that java interfaces may change at any time. I tend to prefer to make a
breaking change in this case and simply add the new parameter to the old
method. Old implementations will be able to catch that at CI time and the fix
is rather trivial. The benefit is simpler Polaris code base and easier OSS code
maintenance (which has more exposure than private implementations).
Apologies if this was already considered and I missed it. This is just my
personal opinion. Please consider it non-blocking.
--
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]
