You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ForeverAngry (via GitHub)" <gi...@apache.org> on 2025/09/04 22:58:26 UTC
[PR] Expire snapshot thread safety issue 2409 [iceberg-python]
ForeverAngry opened a new pull request, #2430:
URL: https://github.com/apache/iceberg-python/pull/2430
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
Closes #2409
# Rationale for this change
This PR fixes a critical thread safety issue in the `ExpireSnapshots` class where concurrent snapshot expiration operations on different tables would share snapshot IDs, causing operations to fail with "snapshot does not exist" errors.
**Root Cause:**
The `ExpireSnapshots` class had class-level attributes (`_snapshot_ids_to_expire`, `_updates`, `_requirements`) that were shared across all instances. When multiple threads created different `ExpireSnapshots` instances, they all shared the same underlying `set()` object for tracking snapshot IDs.
**Impact:**
- Thread 1: `table1.expire_snapshots().by_id(1001)` adds `1001` to shared set
- Thread 2: `table2.expire_snapshots().by_id(2001)` adds `2001` to same shared set
- Result: Both threads have `{1001, 2001}` and try to expire snapshot `1001` from `table2`, causing failure
**Solution:**
Moved the shared class-level attributes to instance-level attributes in the `__init__` method, ensuring each `ExpireSnapshots` instance has its own isolated state.
## Are these changes tested?
Yes, comprehensive test coverage has been added:
- **`test_thread_safety_fix()`** - Verifies that different ExpireSnapshots instances have separate snapshot sets
- **`test_concurrent_operations()`** - Tests concurrent operations don't contaminate each other
- **`test_concurrent_different_tables_expiration()`** - Reproduces the exact scenario from GitHub issue #2409
- **`test_concurrent_same_table_different_snapshots()`** - Tests concurrent operations on the same table
- **`test_cross_table_snapshot_id_isolation()`** - Validates no cross-contamination of snapshot IDs between tables
- **`test_batch_expire_snapshots()`** - Tests batch expiration operations in threaded environments
All existing tests continue to pass, ensuring no regression in functionality.
## Are there any user-facing changes?
**No breaking changes.** The public API remains identical:
- All existing `ExpireSnapshots` methods work the same way
- Method signatures are unchanged
- Behavior is identical except for the thread safety fix
**Behavioral improvement:**
- Concurrent `expire_snapshots()` operations on different tables now work correctly
- No more "snapshot does not exist" errors when using ExpireSnapshots in multi-threaded environments
This is a pure bug fix with no user-facing API changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2377320115
##########
tests/table/test_expire_snapshots.py:
##########
@@ -223,3 +227,384 @@ def test_expire_snapshots_by_ids(table_v2: Table) -> None:
assert EXPIRE_SNAPSHOT_1 not in remaining_snapshots
assert EXPIRE_SNAPSHOT_2 not in remaining_snapshots
assert len(table_v2.metadata.snapshots) == 1
+
+
+def test_thread_safety_fix() -> None:
Review Comment:
Your saying this is the one, good test, right? :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot mutability issue [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2381615448
##########
tests/table/test_expire_snapshots.py:
##########
@@ -223,3 +227,384 @@ def test_expire_snapshots_by_ids(table_v2: Table) -> None:
assert EXPIRE_SNAPSHOT_1 not in remaining_snapshots
assert EXPIRE_SNAPSHOT_2 not in remaining_snapshots
assert len(table_v2.metadata.snapshots) == 1
+
+
+def test_thread_safety_fix() -> None:
Review Comment:
Yes, indeed :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2342550865
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -924,9 +924,12 @@ class ExpireSnapshots(UpdateTableMetadata["ExpireSnapshots"]):
Pending changes are applied on commit.
"""
- _snapshot_ids_to_expire: Set[int] = set()
- _updates: Tuple[TableUpdate, ...] = ()
- _requirements: Tuple[TableRequirement, ...] = ()
+ def __init__(self, transaction: Transaction) -> None:
+ super().__init__(transaction)
+ # Initialize instance-level attributes to avoid sharing state between instances
+ self._snapshot_ids_to_expire: Set[int] = set()
+ self._updates: Tuple[TableUpdate, ...] = ()
+ self._requirements: Tuple[TableRequirement, ...] = ()
Review Comment:
Understood!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "smaheshwar-pltr (via GitHub)" <gi...@apache.org>.
smaheshwar-pltr commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2328802247
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -924,9 +924,12 @@ class ExpireSnapshots(UpdateTableMetadata["ExpireSnapshots"]):
Pending changes are applied on commit.
"""
- _snapshot_ids_to_expire: Set[int] = set()
- _updates: Tuple[TableUpdate, ...] = ()
- _requirements: Tuple[TableRequirement, ...] = ()
+ def __init__(self, transaction: Transaction) -> None:
+ super().__init__(transaction)
+ # Initialize instance-level attributes to avoid sharing state between instances
+ self._snapshot_ids_to_expire: Set[int] = set()
+ self._updates: Tuple[TableUpdate, ...] = ()
+ self._requirements: Tuple[TableRequirement, ...] = ()
Review Comment:
Good catch!
Nit: I'd personally keep class-level _annotations_ here (with _assignment_ in the constructor, so state still isn't shared), so the code would look similar to what we have for `Transaction`: https://github.com/apache/iceberg-python/blob/52d810efb62e39ec6d8d6a2f4cd2cad8165e2d2c/pyiceberg/table/__init__.py#L247-L263
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2331522547
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -924,9 +924,12 @@ class ExpireSnapshots(UpdateTableMetadata["ExpireSnapshots"]):
Pending changes are applied on commit.
"""
- _snapshot_ids_to_expire: Set[int] = set()
- _updates: Tuple[TableUpdate, ...] = ()
- _requirements: Tuple[TableRequirement, ...] = ()
+ def __init__(self, transaction: Transaction) -> None:
+ super().__init__(transaction)
+ # Initialize instance-level attributes to avoid sharing state between instances
+ self._snapshot_ids_to_expire: Set[int] = set()
+ self._updates: Tuple[TableUpdate, ...] = ()
+ self._requirements: Tuple[TableRequirement, ...] = ()
Review Comment:
💯
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot mutability issue [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "QlikFrederic (via GitHub)" <gi...@apache.org>.
QlikFrederic commented on PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#issuecomment-3258118485
Tried this change out in code where we are expiring snapshots from 2 iceberg tables in separate threads and all is working fine now. 👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2347670606
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -924,9 +924,12 @@ class ExpireSnapshots(UpdateTableMetadata["ExpireSnapshots"]):
Pending changes are applied on commit.
"""
- _snapshot_ids_to_expire: Set[int] = set()
- _updates: Tuple[TableUpdate, ...] = ()
- _requirements: Tuple[TableRequirement, ...] = ()
+ def __init__(self, transaction: Transaction) -> None:
+ super().__init__(transaction)
+ # Initialize instance-level attributes to avoid sharing state between instances
+ self._snapshot_ids_to_expire: Set[int] = set()
+ self._updates: Tuple[TableUpdate, ...] = ()
+ self._requirements: Tuple[TableRequirement, ...] = ()
Review Comment:
@smaheshwar-pltr i applied the changes
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "smaheshwar-pltr (via GitHub)" <gi...@apache.org>.
smaheshwar-pltr commented on code in PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#discussion_r2328802247
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -924,9 +924,12 @@ class ExpireSnapshots(UpdateTableMetadata["ExpireSnapshots"]):
Pending changes are applied on commit.
"""
- _snapshot_ids_to_expire: Set[int] = set()
- _updates: Tuple[TableUpdate, ...] = ()
- _requirements: Tuple[TableRequirement, ...] = ()
+ def __init__(self, transaction: Transaction) -> None:
+ super().__init__(transaction)
+ # Initialize instance-level attributes to avoid sharing state between instances
+ self._snapshot_ids_to_expire: Set[int] = set()
+ self._updates: Tuple[TableUpdate, ...] = ()
+ self._requirements: Tuple[TableRequirement, ...] = ()
Review Comment:
Good catch!
Nit: I'd personally keep class-level annotations here (with assignment in the constructor, so state still shouldn't be shared), so the code would look similar to what we have for `Transaction`: https://github.com/apache/iceberg-python/blob/52d810efb62e39ec6d8d6a2f4cd2cad8165e2d2c/pyiceberg/table/__init__.py#L247-L263
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Expire snapshot thread safety issue 2409 [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on PR #2430:
URL: https://github.com/apache/iceberg-python/pull/2430#issuecomment-3263306332
> Tried this change out in code where we are expiring snapshots from 2 iceberg tables in separate threads and all is working fine now. 👍
Thanks for testing it!!! Let me know if you bump into any other issues.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org