dennishuo commented on code in PR #1026:
URL: https://github.com/apache/polaris/pull/1026#discussion_r2004751161
##########
spec/polaris-management-service.yml:
##########
@@ -850,9 +850,92 @@ components:
- $ref: "#/components/schemas/Catalog"
- type: object
properties:
- remoteUrl:
- type: string
- description: URL to the remote catalog API
+ connectionConfigInfo:
Review Comment:
Yeah normally I'd be more hesitant to make breaking changes, but in this
case there wasn't any code in Polaris actually using it.
It's conceivable that someone who customized Polaris for their own services
uses it, so maybe we could see if anyone vetoes it, but in general it doesn't
really make sense to have a `remoteUrl` without connection authentication
settings anyways. At *best* it would be used as a cosmetic string to display
somewhere.
I've at least checked that one of the large stakeholders of a customized
Polaris deployment (Snowflake OpenCatalog) does not use it.
##########
spec/polaris-management-service.yml:
##########
@@ -850,9 +850,92 @@ components:
- $ref: "#/components/schemas/Catalog"
- type: object
properties:
- remoteUrl:
- type: string
- description: URL to the remote catalog API
+ connectionConfigInfo:
+ $ref: "#/components/schemas/ConnectionConfigInfo"
+
+ ConnectionConfigInfo:
+ type: object
+ description: A connection configuration representing a remote catalog
service
+ properties:
+ connectionType:
+ type: string
+ enum:
+ - ICEBERG_REST
+ description: The type of remote catalog service represented by this
connection
+ uri:
+ type: string
+ description: URI to the remote catalog service
+ required:
+ - connectionType
+ discriminator:
+ propertyName: connectionType
+ mapping:
+ ICEBERG_REST: "#/components/schemas/IcebergRestConnectionConfigInfo"
+
+ IcebergRestConnectionConfigInfo:
Review Comment:
This one is specifically for the Iceberg REST-specific subclass, where the
`discriminator connectionType` is `ICEBERG_REST`. I suppose if all different
connection types end up needing the same set of parameters then we don't need
to use `discriminator`-based subclasses and could just make the enum function
by itself. Though if we then find out there are type-specific fields we'd need
to basically add an "optional" of each possible specialized connection type if
we didn't start out with the discriminator approach.
Anyways, right now the Iceberg REST connection does have the
`remoteCatalogName` which is intended to be used in a way that is somewhat
specific to Iceberg REST -- in particular, to pass it in as the `warehouse`
property when calling `getConfig` and possibly expecting an override of the URL
`PREFIX`. If this old-fashioned handshake is deprecated someday, then `PREFIX`
would need to become first-class and again it would be specific to the way
Iceberg REST constructs paths.
--
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]
