Skip to content

Expire snapshot thread safety issue 2409 #2430

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Expire snapshot thread safety issue 2409 #2430

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
621b43f
Fix thread safety in ExpireSnapshots by initializing instance-level a…
ForeverAngry Sep 4, 2025
4565968
Fix thread safety in ExpireSnapshots by ensuring isolated state acros…
ForeverAngry Sep 4, 2025
c8b6530
Refactored the approach for initializing instance-level attributes
ForeverAngry Sep 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions pyiceberg/table/update/snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, ...] = ()
Copy link
Contributor

@smaheshwar-pltr smaheshwar-pltr Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

iceberg-python/pyiceberg/table/__init__.py

Lines 247 to 263 in 52d810e

class Transaction:
_table: Table
_autocommit: bool
_updates: Tuple[TableUpdate, ...]
_requirements: Tuple[TableRequirement, ...]
def __init__(self, table: Table, autocommit: bool = False):
"""Open a transaction to stage and commit changes to a table.
Args:
table: The table that will be altered.
autocommit: Option to automatically commit the changes when they are staged.
"""
self._table = table
self._autocommit = autocommit
self._updates = ()
self._requirements = ()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smaheshwar-pltr i applied the changes


def _commit(self) -> UpdatesAndRequirements:
"""
Expand Down
Loading