dimas-b commented on code in PR #3826:
URL: https://github.com/apache/polaris/pull/3826#discussion_r2897096533


##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +264,53 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessConfig:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
configuration is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
configurations of the same type are available.

Review Comment:
   for the sake of clarity, please specify the behavior when several prefixes 
have the same length.
   
   I suggest using the one that goes first in the list of config objects.



##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +264,53 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessConfig:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
configuration is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
configurations of the same type are available.
+        config:
+          type: object
+          description: |
+            Storage access configurations for AWS S3, GCP GCS, and Azure ADLS 
are supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## S3 Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3
+              - `s3.access-key-id`: id for credentials that provide access to 
the data in S3
+              - `s3.secret-access-key`: secret for credentials that provide 
access to data in S3
+              - `s3.session-token`: if present, this value should be used for 
as the session token
+              - `s3.session-token-expires-at-ms`: the time the aws session 
token expires, in milliseconds
+            Extra properties:

Review Comment:
   What makes there properties "extra"? Do we even have to categorize 
properties into "main" and "extra"?
   
   I propose describing all properties uniformly because clients will need to 
process them all.



##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +262,55 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessCredential:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
credential is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
credentials of the same type are available.
+        config:
+          type: object
+          description: |
+            Credential configurations for AWS S3, GCP GCS, and Azure ADLS are 
supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## AWS Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3

Review Comment:
   Hmmm... this line still mentions "AWS" 🤔 I'd propose: `[...] tables stored 
in S3-compatible systems` (also on line 281).



-- 
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]

Reply via email to