dimas-b commented on code in PR #1557:
URL: https://github.com/apache/polaris/pull/1557#discussion_r2082773552
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisOverlappingTableTest.java:
##########
@@ -79,15 +79,15 @@ static Stream<Arguments> testTableLocationRestrictions() {
"ALLOW_UNSTRUCTURED_TABLE_LOCATION", "false",
"ALLOW_TABLE_LOCATION_OVERLAP", "false");
Map<String, Object> laxCatalog =
Map.of(
- ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(),
+ ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfigs().getFirst(),
"true",
- ALLOW_TABLE_LOCATION_OVERLAP.catalogConfig(),
+ ALLOW_TABLE_LOCATION_OVERLAP.catalogConfigs().getFirst(),
Review Comment:
optional suggestion: `.catalogConfigs().getFirst()` -> `.canonicalName()`
(or just keep the old `catalogConfig()` with updated javadoc.
##########
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java:
##########
@@ -92,27 +103,51 @@ public Builder<T> defaultValue(T defaultValue) {
return this;
}
- public Builder<T> catalogConfig(String catalogConfig) {
- this.catalogConfig = Optional.of(catalogConfig);
+ public Builder<T> addCatalogConfig(String catalogConfig) {
+ if (!catalogConfig.startsWith(catalogConfigPrefix)) {
+ throw new IllegalArgumentException(
+ "Catalog configs are expected to start with " +
catalogConfigPrefix);
+ }
+ if (allCatalogConfigs.contains(catalogConfig)) {
+ throw new IllegalArgumentException("Catalog config already exists: " +
catalogConfig);
+ }
+ this.catalogConfigs.add(catalogConfig);
+ allCatalogConfigs.add(catalogConfig);
+ return this;
+ }
+
+ /**
+ * Used to support backwards compatability before there were reserved
properties. Usage of this
+ * method should be removed over time.
+ */
+ @Deprecated
+ public Builder<T> addCatalogConfigUnsafe(String catalogConfig) {
+ LOGGER.info("catalogConfigUnsafe is deprecated! Use catalogConfig()
instead.");
+ if (allCatalogConfigs.contains(catalogConfig)) {
+ throw new IllegalArgumentException("Catalog config already exists: " +
catalogConfig);
+ }
+ this.catalogConfigs.add(catalogConfig);
+ allCatalogConfigs.add(catalogConfig);
Review Comment:
I'm not sure about using static state in the builder methods... If the
builder is never converted to a value class, adding to the static state would
be invalid.
Could we do this validation in the constructor of `PolarisConfiguration`
instead? If you're ok with that, I'd also suggest collecting actual
`PolarisConfiguration` instances and enumerating them for validation (instead
of collecting names). The list vs. map overhead is negligible for our number of
entries, but the logic could be clearer , I hope.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
