Xuanwo commented on issue #1038:
URL: https://github.com/apache/iceberg-rust/issues/1038#issuecomment-2720050597
Before continuing the discussion, I would like to first explain our current
error design goals. In general, error design needs to serve the following
objectives:
- The implementer does not need to do extra work.
- The caller can know what error has occurred.
- The caller can decide how to handle this error.
- The caller can understand why this error occurred.
Iceberg's error provides a clear `ErrorKind`, a concise summary message, and
detailed context. Callers can know what happened, decide how to handle it and
understand why this error occurred. It also reduce our efforts to decide while
error to return while using.
This design shifts our focus from ourselves to our end users. That being
said, we generally do not return errors like `serde_json` failure or `io.EOF`.
Instead, we consider whether the error is meaningful to users and contributes
to their error handling.
I still think it would be better if we had `ErrorKind::TableNotFound`,
categorizing them as `noun-adj`. Regardless of which APIs users are calling,
they would know what happened.
I propose to add:
```diff
pub enum ErrorKind {
Unexpected,
DataInvalid,
FeatureUnsupported,
+ NamespaceNotFound,
+ NamespaceExists,
+ TableNotFound,
+ TableExists,
}
```
---
> Sorry I should have been clear, I meant that there could be a dedicated
Catalog error kind that could be a variant of `ErrorKind`. For example:
Hi, a dedicated catalog error kind doesn't look good to me.
Let's try write code from our users side:
```rust
let table = match ctl.load_table("test").await {
Ok(table) => table,
Err(err) if err.kind() == ErrorKind::TableNotExists => {
// handle table not exists, like gerenate a user-friendly error message
}
Err(err) => {
// unexpected things happend, we need to log or return bacl.
}
}
```
If we have different layers of error kind:
```rust
let table = match ctl.load_table("test").await {
Ok(table) => table,
Err(err) if err.kind() ==
ErrorKind::Catalog(CatalogErrorKind::NoSuchTable) => {
// handle table not exists, like gerenate a user-friendly error message
}
Err(err) => {
// unexpected things happend, we need to log or return bacl.
}
}
```
Users need to be aware that it's a catalog error first. However, it's not
something they need to take action on.
Add more errors variants doesn't serve our error handling goal too.
--
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
