dimas-b commented on code in PR #2802:
URL: https://github.com/apache/polaris/pull/2802#discussion_r2546712505
##########
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](https://github.com/apache/polaris/pull/2012#discussion_r2208655528)
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]
