Copilot commented on code in PR #10689: URL: https://github.com/apache/gravitino/pull/10689#discussion_r3044911641
########## clients/client-python/tests/integration/test_relational_catalog.py: ########## @@ -0,0 +1,311 @@ +# 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. + +import logging +from random import randint + +from gravitino import ( + Catalog, + GravitinoAdminClient, + GravitinoClient, + NameIdentifier, +) +from gravitino.api.rel.column import Column +from gravitino.api.rel.expressions.distributions.distributions import Distributions +from gravitino.api.rel.expressions.transforms.transforms import Transforms +from gravitino.api.rel.indexes.indexes import Indexes +from gravitino.api.rel.table import Table +from gravitino.api.rel.table_change import TableChange +from gravitino.api.rel.types.types import Types +from gravitino.client.relational_table import RelationalTable +from gravitino.exceptions.base import ( + NoSuchSchemaException, + NoSuchTableException, + TableAlreadyExistsException, +) +from gravitino.namespace import Namespace +from tests.integration.containers.hdfs_container import HDFSContainer +from tests.integration.integration_test_env import IntegrationTestEnv + +logger = logging.getLogger(__name__) + + +class TestRelationalCatalog(IntegrationTestEnv): + METALAKE_NAME: str = "TestRelationalTable_metalake" + str(randint(1, 10000)) + CATALOG_NAME: str = "relational_catalog" + CATALOG_PROVIDER: str = "hive" + SCHEMA_NAME: str = "test_schema" + TABLE_NAME: str = "test_table" + TABLE_IDENT: NameIdentifier = NameIdentifier.of(SCHEMA_NAME, TABLE_NAME) + TABLE_COMMENT: str = "Test table for relational catalog" + TABLE_PROPERTIES = {"property1": "value1", "property2": "value2"} + + @classmethod + def setUpClass(cls): + cls.hdfs_container: HDFSContainer = HDFSContainer() + hive_metastore_uri = f"thrift://{cls.hdfs_container.get_ip()}:9083" + logger.info("Started Hive container with metastore URI: %s", hive_metastore_uri) + cls.gravitino_admin_client = GravitinoAdminClient(uri="http://localhost:8090";) Review Comment: `setUpClass` overrides `IntegrationTestEnv.setUpClass` but doesn't call `super().setUpClass()`. This bypasses the shared integration env bootstrapping (starting/validating the Gravitino server) that other integration tests rely on (e.g., `test_relational_table.py` calls `super().setUpClass()`), and will likely cause these tests to fail when the server isn't already running. Call `super().setUpClass()` at the start of this method. ########## clients/client-python/tests/integration/test_relational_catalog.py: ########## @@ -0,0 +1,311 @@ +# 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. + +import logging +from random import randint + +from gravitino import ( + Catalog, + GravitinoAdminClient, + GravitinoClient, + NameIdentifier, +) +from gravitino.api.rel.column import Column +from gravitino.api.rel.expressions.distributions.distributions import Distributions +from gravitino.api.rel.expressions.transforms.transforms import Transforms +from gravitino.api.rel.indexes.indexes import Indexes +from gravitino.api.rel.table import Table +from gravitino.api.rel.table_change import TableChange +from gravitino.api.rel.types.types import Types +from gravitino.client.relational_table import RelationalTable +from gravitino.exceptions.base import ( + NoSuchSchemaException, + NoSuchTableException, + TableAlreadyExistsException, +) +from gravitino.namespace import Namespace +from tests.integration.containers.hdfs_container import HDFSContainer +from tests.integration.integration_test_env import IntegrationTestEnv + +logger = logging.getLogger(__name__) + + +class TestRelationalCatalog(IntegrationTestEnv): + METALAKE_NAME: str = "TestRelationalTable_metalake" + str(randint(1, 10000)) Review Comment: `METALAKE_NAME` uses the prefix `TestRelationalTable_metalake`, which matches the prefix used by `TestRelationalTable` (`clients/client-python/tests/integration/test_relational_table.py:42`). This increases the chance of metalake name collisions when integration tests run together and makes the source of created resources harder to identify. Use a prefix that matches this test class (e.g., `TestRelationalCatalog_metalake`). ```suggestion METALAKE_NAME: str = "TestRelationalCatalog_metalake" + str(randint(1, 10000)) ``` ########## clients/client-python/tests/integration/test_relational_catalog.py: ########## @@ -0,0 +1,311 @@ +# 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. + +import logging +from random import randint + +from gravitino import ( + Catalog, + GravitinoAdminClient, + GravitinoClient, + NameIdentifier, +) +from gravitino.api.rel.column import Column +from gravitino.api.rel.expressions.distributions.distributions import Distributions +from gravitino.api.rel.expressions.transforms.transforms import Transforms +from gravitino.api.rel.indexes.indexes import Indexes +from gravitino.api.rel.table import Table +from gravitino.api.rel.table_change import TableChange +from gravitino.api.rel.types.types import Types +from gravitino.client.relational_table import RelationalTable +from gravitino.exceptions.base import ( + NoSuchSchemaException, + NoSuchTableException, + TableAlreadyExistsException, +) +from gravitino.namespace import Namespace +from tests.integration.containers.hdfs_container import HDFSContainer +from tests.integration.integration_test_env import IntegrationTestEnv + +logger = logging.getLogger(__name__) + + +class TestRelationalCatalog(IntegrationTestEnv): + METALAKE_NAME: str = "TestRelationalTable_metalake" + str(randint(1, 10000)) + CATALOG_NAME: str = "relational_catalog" + CATALOG_PROVIDER: str = "hive" + SCHEMA_NAME: str = "test_schema" + TABLE_NAME: str = "test_table" + TABLE_IDENT: NameIdentifier = NameIdentifier.of(SCHEMA_NAME, TABLE_NAME) + TABLE_COMMENT: str = "Test table for relational catalog" + TABLE_PROPERTIES = {"property1": "value1", "property2": "value2"} + + @classmethod + def setUpClass(cls): + cls.hdfs_container: HDFSContainer = HDFSContainer() + hive_metastore_uri = f"thrift://{cls.hdfs_container.get_ip()}:9083" + logger.info("Started Hive container with metastore URI: %s", hive_metastore_uri) + cls.gravitino_admin_client = GravitinoAdminClient(uri="http://localhost:8090";) + cls.gravitino_admin_client.create_metalake( + cls.METALAKE_NAME, + comment="Test metalake for relational catalog", + properties={}, + ) + cls.gravitino_client: GravitinoClient = GravitinoClient( + uri="http://localhost:8090";, metalake_name=cls.METALAKE_NAME + ) + cls.catalog = cls.gravitino_client.create_catalog( + name=cls.CATALOG_NAME, + catalog_type=Catalog.Type.RELATIONAL, + provider=cls.CATALOG_PROVIDER, + comment="Test relational catalog", + properties={"metastore.uris": hive_metastore_uri}, + ) + + @classmethod + def tearDownClass(cls): + try: + cls.gravitino_client.drop_catalog(name=cls.CATALOG_NAME, force=True) + cls.gravitino_admin_client.drop_metalake(name=cls.METALAKE_NAME, force=True) + except Exception as e: # pylint: disable=broad-exception-caught + logger.warning("Failed to clean up class-level resources: %s", e) + + # Clean up the HDFS/Hive container + if cls.hdfs_container: + try: + cls.hdfs_container.close() + except Exception as e: # pylint: disable=broad-exception-caught + logger.warning("Failed to clean up HDFS container: %s", e) + Review Comment: `tearDownClass` doesn't call `super().tearDownClass()`. If `IntegrationTestEnv` started the Gravitino server for this test class, skipping the base teardown can leave the server running and leak resources across the integration suite. Add `super().tearDownClass()` (typically at the end) to match the pattern used in `test_relational_table.py`. ```suggestion try: cls.gravitino_client.drop_catalog(name=cls.CATALOG_NAME, force=True) cls.gravitino_admin_client.drop_metalake( name=cls.METALAKE_NAME, force=True ) except Exception as e: # pylint: disable=broad-exception-caught logger.warning("Failed to clean up class-level resources: %s", e) # Clean up the HDFS/Hive container if cls.hdfs_container: try: cls.hdfs_container.close() except Exception as e: # pylint: disable=broad-exception-caught logger.warning("Failed to clean up HDFS container: %s", e) finally: super().tearDownClass() ``` -- 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]
