dimas-b commented on code in PR #2802:
URL: https://github.com/apache/polaris/pull/2802#discussion_r2546755750
##########
spec/polaris-management-service.yml:
##########
@@ -1103,6 +1103,16 @@ components:
type: string
description: the aws user arn used to assume the aws role
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
+ currentKmsKey:
+ type: string
+ description: the aws kms key arn used to encrypt s3 data
+ example: "arn:aws:kms::123456789001:key/01234578"
+ allowedKmsKeys:
+ type: array
+ description: The list of kms keys that this catalog and its
clients are allow to use for reading s3 data
Review Comment:
typo: `allow` -> `allowed`
##########
api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java:
##########
@@ -70,6 +71,36 @@ public void testJsonFormat() throws JsonProcessingException {
+ "\"properties\":{\"default-base-location\":\"s3://test/\"},"
+ "\"storageConfigInfo\":{"
+ "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\","
+ + "\"allowedKmsKeys\":[],"
Review Comment:
This is a bit of a concern, unfortunately. (Sorry, I did not notice it
before because of the other test code changes).
This means old clients will see the new allowedKmsKeys and other new
properties (after a server upgrade) even if they were not set in their
catalogs... IMHO, it's a potential backward compatibility issue.
However, our history shows that the community might be ok with that kind of
change.
From my POV, it would be preferable to avoid changes on the wire, if
possible... Meaning that defaults on newly added properties should not be
visible to old clients... WDYT?
If that is too cumbersome, I'd like some more reviewer to express their
opinions whether exposing new properties by default is acceptable.
--
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]
