yj-lee0503 commented on code in PR #3480:
URL: https://github.com/apache/polaris/pull/3480#discussion_r2749545833
##########
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:
Hi @dimas-b . Thank you for the thoughtful feedback (and the kind words). ✨
Also, I appreciate sharing the evolution guidelines.
I’m leaning toward updating the PR to the breaking-change approach: adding
the new parameter directly to the existing interface method and removing the
default-method delegation + WARN, unless @dennishuo, @adutra, or others feel
strongly that we should preserve compatibility for external implementations
right now. I've prepared the changes to be pushed. Please let me know!
Thanks for looping in @dennishuo as well. I look forward to hearing from
you all!. 🙇
--
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]
