Copilot commented on code in PR #10787: URL: https://github.com/apache/gravitino/pull/10787#discussion_r3091113080
########## iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.iceberg.common.ops; + +import java.lang.reflect.Field; +import java.util.Collections; +import org.apache.gravitino.iceberg.common.IcebergConfig; +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.catalog.Namespace; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestIcebergCatalogWrapper { + + @Test + public void testCatalogAndNamespaceShouldBeLazyLoaded() throws Exception { + IcebergCatalogWrapper wrapper = + new IcebergCatalogWrapper(new IcebergConfig(Collections.emptyMap())); + + Assertions.assertNull(getFieldValue(wrapper, "catalog")); + Assertions.assertNull(getFieldValue(wrapper, "asNamespaceCatalog")); + Assertions.assertNull(getFieldValue(wrapper, "metadataCache")); + + Catalog loadedCatalog = wrapper.getCatalog(); + Assertions.assertNotNull(loadedCatalog); + Assertions.assertSame(loadedCatalog, getFieldValue(wrapper, "catalog")); + Assertions.assertNull(getFieldValue(wrapper, "asNamespaceCatalog")); + Assertions.assertNull(getFieldValue(wrapper, "metadataCache")); + + wrapper.namespaceExists(Namespace.of("test_db")); + Assertions.assertNotNull(getFieldValue(wrapper, "asNamespaceCatalog")); + } + + @Test + public void testCloseShouldNotInitializeCatalog() throws Exception { + IcebergCatalogWrapper wrapper = + new IcebergCatalogWrapper(new IcebergConfig(Collections.emptyMap())); + + wrapper.close(); + + Assertions.assertNull(getFieldValue(wrapper, "catalog")); + Assertions.assertNull(getFieldValue(wrapper, "asNamespaceCatalog")); + Assertions.assertNull(getFieldValue(wrapper, "metadataCache")); + } Review Comment: The PR claims lazy initialization of `metadataCache`, but the tests only assert it remains null after `getCatalog()` / namespace calls. There is no assertion that the metadata cache is actually initialized on first table operation (and that `close()` closes it when it has been initialized). Please add a test that triggers metadata-cache initialization (e.g., by configuring `IcebergConfig.TABLE_METADATA_CACHE_IMPL` to a real cache impl and invoking a table operation or reflective call) and verifies the expected lifecycle. ########## iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.iceberg.common.ops; + +import java.lang.reflect.Field; +import java.util.Collections; +import org.apache.gravitino.iceberg.common.IcebergConfig; +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.catalog.Namespace; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestIcebergCatalogWrapper { + + @Test + public void testCatalogAndNamespaceShouldBeLazyLoaded() throws Exception { + IcebergCatalogWrapper wrapper = + new IcebergCatalogWrapper(new IcebergConfig(Collections.emptyMap())); + + Assertions.assertNull(getFieldValue(wrapper, "catalog")); + Assertions.assertNull(getFieldValue(wrapper, "asNamespaceCatalog")); + Assertions.assertNull(getFieldValue(wrapper, "metadataCache")); + + Catalog loadedCatalog = wrapper.getCatalog(); + Assertions.assertNotNull(loadedCatalog); + Assertions.assertSame(loadedCatalog, getFieldValue(wrapper, "catalog")); + Assertions.assertNull(getFieldValue(wrapper, "asNamespaceCatalog")); + Assertions.assertNull(getFieldValue(wrapper, "metadataCache")); + + wrapper.namespaceExists(Namespace.of("test_db")); + Assertions.assertNotNull(getFieldValue(wrapper, "asNamespaceCatalog")); + } Review Comment: These tests assert laziness by reflecting on private fields (`catalog`, `asNamespaceCatalog`, `metadataCache`). This tightly couples the test to implementation details (field names / encapsulation), making refactors needlessly risky. Prefer asserting laziness via observable behavior (e.g., constructing with a deliberately unreachable REST/JDBC URI should succeed, and only fail when `getCatalog()`/operations are invoked), or expose a minimal package-private/`@VisibleForTesting` accessor indicating initialization state. ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java: ########## @@ -95,66 +95,110 @@ public IcebergCatalogWrapper(IcebergConfig icebergConfig) { if (!IcebergCatalogBackend.MEMORY.equals(catalogBackend)) { this.catalogUri = icebergConfig.get(IcebergConfig.CATALOG_URI); } - this.catalog = IcebergCatalogUtil.loadCatalogBackend(catalogBackend, icebergConfig); - if (catalog instanceof SupportsNamespaces) { - this.asNamespaceCatalog = (SupportsNamespaces) catalog; + Map<String, String> catalogPropertiesMap = icebergConfig.getIcebergCatalogProperties(); + this.configuration = FileSystemUtils.createConfiguration(null, catalogPropertiesMap); + } + + public Catalog getCatalog() { + Catalog loadedCatalog = catalog; + if (loadedCatalog != null) { + return loadedCatalog; } - this.metadataCache = loadTableMetadataCache(icebergConfig, catalog); - this.catalogPropertiesMap = icebergConfig.getIcebergCatalogProperties(); - this.configuration = FileSystemUtils.createConfiguration(null, catalogPropertiesMap); + synchronized (initializationLock) { + if (catalog == null) { + catalog = IcebergCatalogUtil.loadCatalogBackend(catalogBackend, icebergConfig); + } + return catalog; + } + } + + public IcebergConfig getIcebergConfig() { + return icebergConfig; + } + + private SupportsNamespaces getNamespaceCatalog() { + SupportsNamespaces namespaceCatalog = asNamespaceCatalog; + if (namespaceCatalog != null) { + return namespaceCatalog; + } + + synchronized (initializationLock) { + if (asNamespaceCatalog == null) { + Catalog loadedCatalog = getCatalog(); + if (loadedCatalog instanceof SupportsNamespaces) { + asNamespaceCatalog = (SupportsNamespaces) loadedCatalog; + } + } + return asNamespaceCatalog; + } + } + + private TableMetadataCache getMetadataCache() { + TableMetadataCache cache = metadataCache; + if (cache != null) { + return cache; + } + + synchronized (initializationLock) { + if (metadataCache == null) { + metadataCache = loadTableMetadataCache(icebergConfig, getCatalog()); + } + return metadataCache; + } } private void validateNamespace(Optional<Namespace> namespace) { namespace.ifPresent( n -> Preconditions.checkArgument(!n.toString().isEmpty(), "Namespace couldn't be empty")); - if (asNamespaceCatalog == null) { + if (getNamespaceCatalog() == null) { throw new UnsupportedOperationException( "The underlying catalog doesn't support namespace operation"); } } private ViewCatalog getViewCatalog() { - if (!(catalog instanceof ViewCatalog)) { - throw new UnsupportedOperationException(catalog.name() + " is not support view"); + Catalog loadedCatalog = getCatalog(); + if (!(loadedCatalog instanceof ViewCatalog)) { + throw new UnsupportedOperationException(loadedCatalog.name() + " is not support view"); Review Comment: The exception message in `getViewCatalog()` is grammatically incorrect and a bit unclear ("is not support view"). Since this line was modified, please update it to a clearer message like "does not support view operations" (and consider including the catalog name/type). ```suggestion throw new UnsupportedOperationException( "The underlying catalog '" + loadedCatalog.name() + "' does not support view operations"); ``` -- 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]
