gh-yzou commented on code in PR #1126:
URL: https://github.com/apache/polaris/pull/1126#discussion_r2002258178


##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogTest.java:
##########
@@ -151,6 +151,8 @@ public Map<String, String> getConfigOverrides() {
           "true",
           
"polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
           "true",
+          "polaris.features.defaults.\"ALLOW_OVERLAPPING_CATALOG_URLS\"",

Review Comment:
   are we doing doing testing for new features?



##########
regtests/setup.sh:
##########
@@ -31,7 +31,7 @@ if [ -z "${SPARK_HOME}" ]; then
 fi
 SPARK_CONF="${SPARK_HOME}/conf/spark-defaults.conf"
 DERBY_HOME="/tmp/derby"
-ICEBERG_VERSION="1.7.1"
+ICEBERG_VERSION="1.8.1"

Review Comment:
   theoretically, the iceberg version used here doesn't have to be aligned with 
the iceberg version  used by Polaris.
   I guess we are always just testing the iceberg version used by the polaris 
service, and the backeward compatibity is guaranteed by iceberg. I am wondering 
how did you find out that this version needs to be updated?  maybe we can add a 
comment in the libs.versions.toml to tell that if the version is updated, also 
update regtests/setup.sh



##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/IcebergHelper.java:
##########
@@ -53,11 +54,11 @@ public static RESTCatalog restCatalog(
             .put(
                 org.apache.iceberg.CatalogProperties.FILE_IO_IMPL,
                 "org.apache.iceberg.inmemory.InMemoryFileIO")
-            .put("warehouse", catalog)
+            .put("warehouse", warehouse)

Review Comment:
   I believe the warehouse name is the catalog name, is there anything that is 
changing with 1.8.1?



##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -215,6 +215,10 @@ public void initialize(String name, Map<String, String> 
properties) {
         name,
         this.catalogName);
 
+    // Ensure catalogProperties is assigned before calling metricsReporter() 
for proper
+    // functionality.
+    catalogProperties = properties;

Review Comment:
   Curious, why it wasn't failing before?



##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogViewTest.java:
##########
@@ -199,7 +204,12 @@ public void before(TestInfo testInfo) {
     this.catalog.initialize(
         CATALOG_NAME,
         ImmutableMap.of(
-            CatalogProperties.FILE_IO_IMPL, 
"org.apache.iceberg.inmemory.InMemoryFileIO"));
+            CatalogProperties.FILE_IO_IMPL,
+            "org.apache.iceberg.inmemory.InMemoryFileIO",
+            CatalogProperties.VIEW_DEFAULT_PREFIX + "key1",

Review Comment:
   what are those keys used for? it seems not needed before



-- 
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]

Reply via email to