You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2024/11/11 02:53:30 UTC
[PR] Fix 399 [polaris]
MonkeyCanCode opened a new pull request, #443:
URL: https://github.com/apache/polaris/pull/443
# Description
This PR fixed issue where ClassCastException can raise when a change made to SUPPORTED_CATALOG_STORAGE_TYPES in `polaris-server.yml`. The exception is due to `List.of` will be using internal `ImmutableCollections` type which can be `List12` for shorter list and `ListN` for longer list. As `config.cast` expects a specific type, it may be better to not use `List.of`. For this PR, I am switching to use `Arraylist` instead to avoid this unexpected error from happening when an valid change is made to the server config.
Fixes https://github.com/apache/polaris/issues/399
## Type of change
Please delete options that are not relevant.
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation update
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
# How Has This Been Tested?
General test suit:
```
➜ polaris git:(fix_399) ✗ ./gradlew clean test
Configuration on demand is an incubating feature.
> Task :polaris-service:openApiGenerate
user-defined server variable support is experimental.
################################################################################
# Thanks for using OpenAPI Generator. #
# Please consider donation to help us maintain this project 🙏 #
# https://opencollective.com/openapi_generator/donate #
################################################################################
Successfully generated code to /Users/yong/Desktop/git/polaris/polaris-service/build/generated
BUILD SUCCESSFUL in 4s
35 actionable tasks: 15 executed, 11 from cache, 9 up-to-date
```
happy path:
```
# start app in terminal 1
➜ polaris git:(fix_399) ✗ ./gradlew runApp
# create catalog in terminal 2
➜ polaris git:(fix_399) ✗ export CLIENT_ID=xxxxx
➜ polaris git:(fix_399) ✗ export CLIENT_SECRET=xxxxx
➜ polaris git:(fix_399) ✗ ./polaris catalogs create --storage-type file --default-base-location file:///tmp/test quickstart_catalog
➜ polaris git:(fix_399) ✗ ./polaris catalogs list
{"type": "INTERNAL", "name": "quickstart_catalog", "properties": {"default-base-location": "file:///tmp/test"}, "createTimestamp": 1731291707237, "lastUpdateTimestamp": 1731291707237, "entityVersion": 1, "storageConfigInfo": {"storageType": "FILE", "allowedLocations": ["file:///tmp/test"]}}
```
original reported unhappy path (fixed now...so also happy path):
```
# update featureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES to only keep `FILE`:
featureConfiguration:
ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING: false
SUPPORTED_CATALOG_STORAGE_TYPES:
- FILE
# start app in terminal 1
➜ polaris git:(fix_399) ✗ ./gradlew runApp
# create catalog in terminal 2
➜ polaris git:(fix_399) ✗ export CLIENT_ID=xxxxx
➜ polaris git:(fix_399) ✗ export CLIENT_SECRET=xxxxx
➜ polaris git:(fix_399) ✗ ./polaris catalogs create --storage-type file --default-base-location file:///tmp/test quickstart_catalog
➜ polaris git:(fix_399) ✗ ./polaris catalogs list
{"type": "INTERNAL", "name": "quickstart_catalog", "properties": {"default-base-location": "file:///tmp/test"}, "createTimestamp": 1731291887163, "lastUpdateTimestamp": 1731291887163, "entityVersion": 1, "storageConfigInfo": {"storageType": "FILE", "allowedLocations": ["file:///tmp/test"]}}
```
# Checklist:
Please delete options that are not relevant.
- [x] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 399 [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #443:
URL: https://github.com/apache/polaris/pull/443#discussion_r1836035287
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -167,12 +168,11 @@ public static <T> Builder<T> builder() {
.key("SUPPORTED_CATALOG_STORAGE_TYPES")
.catalogConfig("supported.storage.types")
.description("The list of supported storage types for a catalog")
- .defaultValue(
- List.of(
+ .defaultValue(new ArrayList<>(List.of(
Review Comment:
Thanks for review. Please take another look when you get a chance to see if that is what you are looking for.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 399 [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard merged PR #443:
URL: https://github.com/apache/polaris/pull/443
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 399 [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #443:
URL: https://github.com/apache/polaris/pull/443#discussion_r1835999169
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -167,12 +168,11 @@ public static <T> Builder<T> builder() {
.key("SUPPORTED_CATALOG_STORAGE_TYPES")
.catalogConfig("supported.storage.types")
.description("The list of supported storage types for a catalog")
- .defaultValue(
- List.of(
+ .defaultValue(new ArrayList<>(List.of(
Review Comment:
Since the PolarisConfiguration is of the type `List`, can we push the wrapping down into the `defaultValue` method?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org