liurenjie1024 commented on code in PR #1383:
URL: https://github.com/apache/iceberg-rust/pull/1383#discussion_r2113609034
##########
crates/iceberg/src/error.rs:
##########
@@ -134,6 +134,8 @@ pub struct Error {
source: Option<anyhow::Error>,
backtrace: Backtrace,
+
+ retryable: bool,
Review Comment:
> For example, Unexpected error can be permanent and PreconditionFailed
error can be temporary.
I don't quite understand why `PreconditionFailed` can be temporrary. For
example, `PreconditionFailed` could be invalid input from user, and it should
be `permanent`.
> I think the basic idea here is to let retryable or temporary property
orthogonal of ErrorKind.
I'm still confusing about this part. Let's think about the use case, usually
the caller of a function determines whether they need to retry, depending on
the return error. For example, the user could decide to retry strategy when
they face network timeout failure.
If we add the `status` or `temporary` field, it's the function telling its
caller whether they should retry. My concern is that adding such a field a
general purpose `Error` could be misleading or confusing. If we add this field
into a concrete `ErrorKind` variant such as `CommitConflict(bool)`, it makes
more sense to me.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
