Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 551
Fix issue 2530#3688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Fix issue 2530 #3688
Changes from all commits
83f789d01f3b5669c5a957281f05c8db5495f0fe3c9cf77b008f6fb45af30d56b110a7f555e5120d352c9b5680aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -124,6 +124,8 @@ dev = [ | ||
| "papermill>=2.6.0", | ||
| "nbformat>=5.10.0", | ||
| "ipykernel>=6.29.0", | ||
| "ruff>=0.15.22", | ||
| "pre-commit>=4.6.0", | ||
| ] | ||
| # for mkdocs | ||
| docs = [ | ||
| @@ -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 | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment doesn't seem to be doing anything? The old comment was better. | ||
| # and requires a CI environment with proper isolation to test subprocess lifecycle and cleanup behavior. | ||
| # See issue #2530: the SQLAlchemy connection pool warning (sqlite) has been fixed by properly closing | ||
| # connections in test fixtures, but ray subprocess warnings still require CI-level testing. | ||
| "ignore:unclosed file:ResourceWarning", | ||
| "ignore:subprocess.*is still running:ResourceWarning", | ||
| # Ignore google-crc32c C extension missing warning (common on Python 3.14+) | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you simplify this comment? This is really verbose and it's hard to understand what the additional text is adding.