Skip to content

feat: add retryable property for Error #1383

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 2 commits into
base: main
Choose a base branch
from
Open

feat: add retryable property for Error #1383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f94e9b
add retryable property for Error
May 27, 2025
2191258
fix spark version
May 27, 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
3 changes: 2 additions & 1 deletion crates/catalog/rest/src/catalog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,8 @@ impl Catalog for RestCatalog {
return Err(Error::new(
ErrorKind::Unexpected,
"CommitFailedException, one or more requirements failed. The client may retry.",
));
)
.with_retryable(true));
}
StatusCode::INTERNAL_SERVER_ERROR => {
return Err(Error::new(
Expand Down
10 changes: 10 additions & 0 deletions crates/iceberg/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ pub struct Error {

source: Option<anyhow::Error>,
backtrace: Backtrace,

retryable: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite confusing. Whether it's retryable should be determined by consumer by ErrorKind.

Copy link
Member

@Xuanwo Xuanwo May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Whether it's retryable should be determined by consumer by ErrorKind.

My current idea is to introduce an ErrorStatus:

  • temporary: This error is temporary, and users can retry the operation if they want.
  • permanent: This error is permanent, so users can't retry it. Even if they do, it's highly likely that it won't work.

In this way, we convey the status, which is only known to us and is orthogonal of ErrorKind instead of the final decision, retryable.

For example, Unexpected error can be permanent and PreconditionFailed error can be temporary.

This is also very useful in cases like our REST catalog client. We can offer built-in retry support and convert a temporary error into a permanent one after several retry attempts.

I also think it would be a good idea to use a temporary: bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite confusing. Whether it's retryable should be determined by consumer by ErrorKind.

temporary: bool +1. I think the basic idea here is to let retryable or temporary property orthogonal of ErrorKind.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

This example makes sense to me.🤔 There are two kinds of retry process: 1. commit conflict, 2. other(maybe network timeout as @liurenjie1024 says). It seems that in the SDK, at least for now, we only try to retry the process when we meet CommitConflict. (In Java, exceptions only retry when they meet CommitFailedException). For other errors, maybe it should be determined by the user. And we can determine whether need to let retry orthogonal of ErrorKind until we have multiple kinds of errors that need to be retried?

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand why PreconditionFailed can be temporrary.

In cases like concurrent commit. Users can retry until this operation succeed or give up while retry times reached.

Take S3 as an example:

https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html

ConditionalRequestConflict

A conflicting operation occurred. If using PutObject you can retry the request. If using multipart upload you should initiate another CreateMultipartUpload request and re-upload each part.

If we add the status or temporary field, it's the function telling its caller whether they should retry.

I think that's exactly why I want to use "temporary" here. We're telling users that this error is temporary, allowing them to decide how to proceed based on their own context.

A temporary error doesn't necessarily mean it should be retried.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In cases like concurrent commit. Users can retry until this operation succeed or give up while retry times reached.

Shouldn't this return by the catalog client as a CommitConflict?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand the benefits of having an ErrorStatus within Error to define if an error is temporary. This basically means we can mark any ErrorKind as "retryable". Maybe there is any concrete example?

On the other hand, I think adding a dedicated ErrorKind like CommitFailed for commit conflicts doesn't conflict with the idea of having an ErrorStatus---we can have both if necessary. Opened #1452 based on this thought, would be happy to hear your thoughts as well!

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, I think adding a dedicated ErrorKind like CommitFailed for commit conflicts doesn't conflict with the idea of having an ErrorStatus

I don't think we should have an error type called CommitFailed, since users can't really do anything about it.

I still think it would be better to add a status or temporary field to Error to make it more orthogonal than ErrorKind.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm supportive for a CommitConflict error kind, as this align better with rest spec: https://github.com/apache/iceberg/blob/8ed1c216503b7193924ca57bd2694025660ac02c/open-api/rest-catalog-open-api.yaml#L1375.

I still think it would be better to add a status or temporary field to Error to make it more orthogonal than ErrorKind.

As we discussed in last sync, this basically means that for some error kind, sometimes it's retryable, and sometimes it's not. I can't come up with such a case, is there any concrete example?

}

impl Display for Error {
Expand Down Expand Up @@ -225,9 +227,17 @@ impl Error {
// `Backtrace::capture()` will check if backtrace has been enabled
// internally. It's zero cost if backtrace is disabled.
backtrace: Backtrace::capture(),

retryable: false,
}
}

/// Set retryable of the error.
pub fn with_retryable(mut self, retryable: bool) -> Self {
self.retryable = retryable;
self
}

/// Add more context in error.
pub fn with_context(mut self, key: &'static str, value: impl Into<String>) -> Self {
self.context.push((key, value.into()));
Expand Down
2 changes: 1 addition & 1 deletion crates/integration_tests/testdata/spark/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ ENV PYTHONPATH=$SPARK_HOME/python:$SPARK_HOME/python/lib/py4j-0.10.9.7-src.zip:$
RUN mkdir -p ${HADOOP_HOME} && mkdir -p ${SPARK_HOME} && mkdir -p /home/iceberg/spark-events
WORKDIR ${SPARK_HOME}

ENV SPARK_VERSION=3.5.5
ENV SPARK_VERSION=3.5.6
ENV ICEBERG_SPARK_RUNTIME_VERSION=3.5_2.12
ENV ICEBERG_VERSION=1.6.0

Expand Down
Loading