Buktoria commented on code in PR #330:
URL: https://github.com/apache/iceberg-python/pull/330#discussion_r1474754856
##########
pyiceberg/table/__init__.py:
##########
@@ -994,6 +1065,7 @@ def refs(self) -> Dict[str, SnapshotRef]:
"""Return the snapshot references in the table."""
return self.metadata.refs
+ @table_commit_retry("properties")
Review Comment:
My take on this is that if we try to implement retries in multiple places it
will make it harder to prevent compounding retries, and thus result in a
multiplying effect in the total number of retries, which is not a behaviour we
want.
I would argue we should have just one place where we apply retries and it
should keep the state of the number of attempts.
We could catch multiple errors here instead of just `CommitFailedException`
to account for network errors.
In terms of accounting for table changes, that is a good point I had not
originally considered. If the table has changed we would continue seeing the
same error, in which case we need to perform a `refresh`. Refresh is an
operation on the `Table` class, not the `Catalog`. So we need to have access to
`Table`. With the current positioning of the retry decorator, we have access to
the table instance so we can refresh the table.
What we can do is after every attempt we `refresh` the table. This is not
optimal in the case where we failed due to network issues, since the table may
have not changed. I would argue that the penalty that we are taking, in this
case, would be minimal and is more than enough to pay for a more simplistic
approach.
--
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]
