Add pathStyleAccess to AwsStorageConfigInfo
#2012
Conversation
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public AwsStorageConfigurationInfo( | ||
| @Nonnull StorageType storageType, | ||
| @Nonnull List<String> allowedLocations, | ||
| @Nonnull String roleARN, | ||
| @Nullable String region) { | ||
| this(storageType, allowedLocations, roleARN, null, region, null, null); | ||
| this(storageType, allowedLocations, roleARN, null, region, null, null, false); |
There was a problem hiding this comment.
We could use the default here, or just leave it null to leverage the default used in orElse. Better not to proliferate the constants around like this.
There was a problem hiding this comment.
changed to use PATH_STYLE_ACCESS_DEFAULT
There was a problem hiding this comment.
Now that we have a default at the API level, is this actually nullable?
There was a problem hiding this comment.
I had to remove the API default.
| type: boolean | ||
| description: >- | ||
| Whether S3 requests to files in this catalog should use 'path-style addressing for buckets'. | ||
| Default: false. |
There was a problem hiding this comment.
I believe OpenAPI actually supports default values, maybe we could just use that?
There was a problem hiding this comment.
Added default: false, changed example to true.
There was a problem hiding this comment.
Actually I had to roll this back. Using Open API defaults breaks strict backward compatibility because is causes the attribute with a default value to be always returned to clients (even when not in use) - CI caught this.
There was a problem hiding this comment.
What exactly was the failure in CI? The client would fail to parse a response because it didn't recognize the property?
There was a problem hiding this comment.
CatalogSerializationTest detected a change in the JSON text (when the new properties are not set)
There was a problem hiding this comment.
@eric-maynard : your previous comment made me realise that the new AwsStorageConfigurationInfo properties are not properly converted back to AwsStorageConfigInfo yet. I'll tackle that as a separate PR, if you do not mind.
I'll convert pathStyleAccess to object Boolean in AwsStorageConfigurationInfo to ensure unset values are distinct from defaults (and thus do not affect older catalogs).
There was a problem hiding this comment.
Oh, I think that's okay to defer.
But...
As long as pathStyleAccess is null in AwsStorageConfigInfo clients will not see it.
That's only true if you don't create a new catalog. So take the following pseudocode:
String catalog = ...createNewCatalog().toString
assert(catalog == "{...}")
We don't have a test like this in CatalogSerializationTest, but if we had one this PR would break it. Personally I don't think this really constitutes a breaking change, but I do think the proper way to handle a default in the API is with a default in the spec rather than a blurb in the description.
There was a problem hiding this comment.
I agree that we do not have a specific test for new property values in REST responses. However, CatalogSerializationTest makes sure defaults in new properties in AwsStorageConfigInfo do not show up in JSON.
As I mentioned above, we're missing code for new properties to be propagated from AwsStorageConfigurationInfo to AwsStorageConfigInfo, so I cannot even add a test for that without extra changes, which is what I'm proposing to do in a follow-up PR.
There was a problem hiding this comment.
actually, never mind, I'll expand this PR a bit :)
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
Outdated
Show resolved
Hide resolved
|
I had to remove one commit (discussed in the API yaml comment thread). PTAL. |
github-project-automation
bot
moved this from Ready to merge
to PRs In Progress
in Basic Kanban Board
This change allows configuring the "path-style" access mode in S3 clients (both in Polaris Servers and Iceberg REST Catalog API clients). This change is applicable both to AWS storage and to non-AWS S3-compatible storage (apache#1530). The Management API change is backward-compatible.
This change allows configuring the "path-style" access mode in S3 clients (both in Polaris Servers and Iceberg REST Catalog API clients).
This change is applicable both to AWS storage and to non-AWS S3-compatible storage (#1530).
The Management API change is backward-compatible.
Includes a fix to AWS config object round-trip via generated API classes.
Dev ML discussion: https://lists.apache.org/thread/oxy2yd4mbq06zr4z57lfptgy95c3lyhb