rambleraptor commented on code in PR #3688:
URL: https://github.com/apache/iceberg-python/pull/3688#discussion_r3660596886
##########
vendor/hive_metastore/ThriftHiveMetastore.py:
##########
@@ -12201,9 +12201,9 @@ def __init__(self, handler):
self._processMap["truncate_table_req"] =
Processor.process_truncate_table_req
self._processMap["get_tables"] = Processor.process_get_tables
self._processMap["get_tables_by_type"] =
Processor.process_get_tables_by_type
- self._processMap[
- "get_all_materialized_view_objects_for_rewriting"
- ] = Processor.process_get_all_materialized_view_objects_for_rewriting
+ self._processMap["get_all_materialized_view_objects_for_rewriting"] = (
Review Comment:
Another formatting change. We really shouldn't change vendor code if we can
avoid it.
##########
pyiceberg/catalog/sql.py:
##########
@@ -778,9 +778,25 @@ def close(self) -> None:
"""Close the catalog and release database connections.
This method closes the SQLAlchemy engine and disposes of all
connection pools.
- This ensures that any cached connections are properly closed, which is
especially
- important for blobfuse scenarios where file handles need to be closed
for
- data to be flushed to persistent storage.
+ This is crucial for proper resource cleanup and must be called in test
fixture
+ teardowns to prevent Python 3.13+ ResourceWarnings about unclosed
connections.
+
+ The connection pool lifecycle is managed by SQLAlchemy's engine. When
dispose()
Review Comment:
Can you simplify this comment? This is really verbose and it's hard to
understand what the additional text is adding.
##########
pyproject.toml:
##########
@@ -171,9 +173,10 @@ filterwarnings = [
"error",
# Ignore Python version deprecation warning from google.api_core while we
still support 3.10
"ignore:You are using a Python version.*which Google will stop
supporting:FutureWarning:google.api_core",
- # Python 3.13 sqlite3 module ResourceWarnings for unclosed database
connections
- "ignore:unclosed database in <sqlite3.Connection object*:ResourceWarning",
- # Ignore Ray subprocess cleanup warnings
+ # Ray subprocess cleanup warnings are suppressed because ray is not
installed in local test environments
Review Comment:
This comment doesn't seem to be doing anything? The old comment was better.
##########
pyiceberg/catalog/sql.py:
##########
@@ -778,9 +778,25 @@ def close(self) -> None:
"""Close the catalog and release database connections.
This method closes the SQLAlchemy engine and disposes of all
connection pools.
- This ensures that any cached connections are properly closed, which is
especially
- important for blobfuse scenarios where file handles need to be closed
for
- data to be flushed to persistent storage.
+ This is crucial for proper resource cleanup and must be called in test
fixture
+ teardowns to prevent Python 3.13+ ResourceWarnings about unclosed
connections.
+
+ The connection pool lifecycle is managed by SQLAlchemy's engine. When
dispose()
+ is called, it closes all idle connections and invalidates active ones.
This is
+ especially important in test environments where fixtures create
catalogs that
+ otherwise may not be explicitly closed, and in production scenarios
with
+ blobfuse where file handles need to be closed for data to be flushed to
+ persistent storage.
+
+ For Python 3.14+, explicit garbage collection is needed to ensure all
+ sqlite3 connections are immediately released, as deferred GC can cause
+ ResourceWarnings during test teardown.
+
+ See: Issue #2530 (https://github.com/apache/iceberg-python/issues/2530)
"""
if hasattr(self, "engine"):
self.engine.dispose()
+ # Force immediate garbage collection to release sqlite3
connections on Python 3.14+
+ import gc
+
+ gc.collect()
Review Comment:
This scares me. Why do we need to force garbage collection to release these
connections? This will have unintended side-effects elsewhere. If not, it'll
just be really slow.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
