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/07/13 01:38:14 UTC
[PR] Feature/add files retry logic 2203 [iceberg-python]
ForeverAngry opened a new pull request, #2205:
URL: https://github.com/apache/iceberg-python/pull/2205
## ✨ Improvements & Features
This PR introduces an enhancement to the existing `add_files` function here:
https://github.com/apache/iceberg-python/blob/dc439402e33d642c9a0c3261f10c078017d6566e/pyiceberg/table/__init__.py#L860:
- 🚦 **Retry Logic for Commits**: Added robust retry handling to `add_files` using the `tenacity` library, improving reliability for concurrent writes and large file operations.
---
## 📝 Rationale for this change
Concurrent writes using `add_files` often result in commit failures requiring expensive recomputation. By adding retry logic directly to the commit process, we avoid unnecessary rebuilding of `DataFile` objects, improving performance and scalability for large or multi-process operations.
---
## ✅ Are these changes tested?
- **Yes.**
- Added unit tests for retry logic to ensure correct handling of commit exceptions.
---
## 🚦 Are there any user-facing changes?
- **Yes.**
- Users have the option to provide tenacity retry kwargs to the `add_files` function arguments.
---
**Closes:** #2203
--
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] Feature/add files retry logic [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3344012166
> While I agree with the need to retry for the `add_files` function, i dont think we should add specific retry logic for this function only. These are all commit operations so it would be more maintainable to address commit retries in general.
>
> The use case you described regarding collecting file stats is useful. I do think we need to improve our retry mechanism that is better than "starting everything from scratch". However, I think we should have separation of concerns; `add_files` shouldnt need to worry about caching file stats.
>
> As a library, pyiceberg aims to provide all the building blocks. We let users implement their own specific solution for these type of optimizations.
Fair enough - following the spirit of providing building blocks, wdyt of doing something like the following, this would expose the functions users need to address the issue, rather than implementing the fix for them:
```python
def _is_valid_object_path(self, path: str) -> bool:
"""
Simple check to validate if a string is a potentially valid object path.
"""
if not isinstance(path, str) or not path.strip():
return False
from urllib.parse import urlparse
parsed = urlparse(path)
if parsed.scheme:
# It's a URL-like path (e.g., s3://, gs://, file://)
return True
else:
return False
def _does_file_exist(self, path: str) -> bool:
"""
Check if a file exists at the given path using the table's IO.
"""
if not self._is_valid_object_path(path):
return False
try:
with self._table.io.new_input(path).open():
return True
except Exception:
return False
def get_data_files_from_objects(self, list_of_objects: List[Union[str, pa.Table]]) -> List[DataFile]:
"""
Convert a list of objects (uris or dataframes) to DataFile objects.
Args:
list_of_objects: List of object paths (strings) or PyArrow tables.
Returns:
List of DataFile objects.
"""
from pyiceberg.io.pyarrow import _dataframe_to_data_files
data_files = []
for obj in list_of_objects:
if isinstance(obj, str) and self._is_valid_object_path(obj):
file_paths = [obj]
data_files.extend(
_parquet_files_to_data_files(
table_metadata=self.table_metadata,
file_paths=file_paths,
io=self._table.io,
)
)
elif isinstance(obj, pa.Table):
## This feature lets users pass DataFrames directly — skipping file I/O — for cases where data is already in memory.
## However, it may cause confusion: some users might expect files to be auto-written, while others assume pre-written files
## are being referenced. Clear docs or explicit parameters (e.g., write_files=) can help set expectations.
data_files.extend(
list(
_dataframe_to_data_files(
table_metadata=self.table_metadata,
write_uuid=uuid.uuid4(),
df=obj,
io=self._table.io,
)
)
)
raise ValueError(f"Unsupported object type: {type(obj)}. Must be a valid path string or PyArrow Table.")
return data_files
def add_data_files(
self, data_files: List[DataFile], snapshot_properties: Dict[str, str] = EMPTY_DICT) -> None:
"""Shorthand API for adding DataFile objects to the table transaction.
"""
if not data_files:
return # No files to add
# Set name mapping if not already set
if self.table_metadata.name_mapping() is None:
self.set_properties(
**{TableProperties.DEFAULT_NAME_MAPPING: self.table_metadata.schema().name_mapping.model_dump_json()}
)
with self.update_snapshot(snapshot_properties=snapshot_properties).fast_append() as update_snapshot:
for data_file in data_files:
update_snapshot.append_data_file(data_file)
```
--
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] Feature/add files retry logic [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3343839287
While I agree with the need to retry for the `add_files` function, i dont think we should add specific retry logic for this function only. These are all commit operations so it would be more maintainable to address commit retries in general.
The use case you described regarding collecting file stats is useful. I do think we need to improve our retry mechanism that is better than "starting everything from scratch". However, I think we should have separation of concerns; `add_files` shouldnt need to worry about caching file stats.
As a library, pyiceberg aims to provide all the building blocks. We let users implement their own specific solution for these type of optimizations.
--
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] Feature/add files retry logic [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3322387707
thanks for your patience with the reviews!
im +1 to what jayce mentioned, i think we'd want to use the existing table retry mechanisms instead of adding retry to a specific user-facing table function.
--
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] Feature/add files retry logic [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry closed pull request #2205: Feature/add files retry logic
URL: https://github.com/apache/iceberg-python/pull/2205
--
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] Feature/add files retry logic [iceberg-python]
Posted by "MrDerecho (via GitHub)" <gi...@apache.org>.
MrDerecho commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3067245177
@ForeverAngry - I looked at this
```python
@retry(
stop=retry_kwargs.get("stop", stop_after_attempt(3)),
wait=retry_kwargs.get("wait", wait_exponential(multiplier=1, min=2, max=10)),
retry=retry_if_exception_type(CommitFailedException),
reraise=True,
)
```
It's been my experience that the lock is resolved very quickly- is it possible to make this seconds (1-10) in lieu of minutes? Some people may want to have these transactions "open until filled" per se- so allowing some configuration, maybe needed. Thanks.
--
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] Feature/add files retry logic [iceberg-python]
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3067263626
I havent explored this part of iceberg upstream yet (java implementation) but it seems like we should implement and take advantage of the `commit.retry` and `commit.status` properties https://iceberg.apache.org/docs/latest/configuration/#table-behavior-properties
--
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] Feature/add files retry logic [iceberg-python]
Posted by "ForeverAngry (via GitHub)" <gi...@apache.org>.
ForeverAngry commented on PR #2205:
URL: https://github.com/apache/iceberg-python/pull/2205#issuecomment-3322452187
> thanks for your patience with the reviews!
>
> im +1 to what jayce mentioned, i think we'd want to use the existing table retry mechanisms instead of adding retry to a specific user-facing table function.
Yeah, @jayceslesar i think using the table properties to set or seed the retry arguments is fine. That being said, i dont think that addresses the issue. The problem here is a bit different, at least from my perspective.
I do think retry logic for **this** function, alone, would actually make sense. The `add_files` method can be used to commit large batches of files. The amount of overhead that goes into collecting that metadata from all of the files in said batch, just to get the point of trying to commit, is what the PR seeks to address. There isnt a good or clean way to pull that metadata prior to calling the `add_files` method to store it _(besides using pyarrow directly),_ and if there were, i dont think there is a public api for `add_datafiles` in pyiceberg _(though i know the function is one of the private methods actually used in one of the functions in the call stack of the `add_files` function)._
From my perspective, the way an `append` is used, in common data pipeline, is likely much different in scope and magnitude, as compared to what the `add_files` function could / typically sees. @kevinjqliu does this context help, and was it persuasive, or is there maybe something im not seeing?
--
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