-
Notifications
You must be signed in to change notification settings - Fork 3k
API, Core: Add 404 handling for /v1/config endpoint #14965
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.
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
base: main
Are you sure you want to change the base?
API, Core: Add 404 handling for /v1/config endpoint #14965
Conversation
Add NoSuchWarehouseException and configErrorHandler to handle 404 responses from the /v1/config endpoint, which can occur when a configured warehouse does not exist. Changes: - Add NoSuchWarehouseException in api module - Add configErrorHandler in ErrorHandlers that throws NoSuchWarehouseException on 404 - Update RESTSessionCatalog to use configErrorHandler for config calls - Update OpenAPI spec to document 404 response for /v1/config - Add unit tests for configErrorHandler
2e41ea1 to
eb7591b
Compare
|
Thanks for raising a corresponding devlist thread, could you include it in the PR description? https://lists.apache.org/thread/xshfmp6m78l4ys6loozwop0fjl0cov3d I'll chime in on the thread 😄 |
|
Thanks @kevinjqliu ! I just updated PR description, at the bottom, to refer to the devlist thread. |
open-api/rest-catalog-open-api.yaml
Outdated
| # Note that this is a representative example response for use as a shorthand in the spec. | ||
| # The fields `message` and `type` as indicated here are not presently prescriptive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Note that this is a representative example response for use as a shorthand in the spec. | |
| # The fields `message` and `type` as indicated here are not presently prescriptive. |
nit: dont really need this explanation since its under example
|
@oguzhanunlu I'm a little concerned around the semantics for a This could result in confusing errors if we aren't handling that correctly. You should add some tests that return a 404 that is not formatted as an error response and ensure that we're surfacing an appropriate error. |
|
Thanks for the feedback @danielcweeks. You're right that a 404 on /v1/config could come from either:
I traced through the error handling flow and have two options: Option 1: Check error.type() != null When DefaultErrorHandler.parseResponse() fails to parse the response body, it returns an ErrorResponse with type = null. Per the spec, type is required, so valid Iceberg servers must include it. if (error.code() == 404 && error.type() != null) {
throw new NoSuchWarehouseException("%s", error.message());
}Pros: Minimal change Option 2: Add explicit Add a boolean field that explicitly tracks whether the response body was successfully parsed. Pros: Explicit, self-documenting, won't break if fallback logic changes Which approach would you prefer? |
|
Hi @danielcweeks, I've addressed your feedback by adding a null check for When the URI points to a non-Iceberg server, the response won't have a valid error type, so we fall through to the default handler instead of throwing NoSuchWarehouseException. I've also added a test for the misconfigured URI case and removed the comment per @kevinjqliu's suggestion. I'd appreciate another look when you have a chance! |
Summary
This PR proposes adding HTTP 404 as a documented response for the
/v1/configendpoint in the REST Catalog OpenAPI specification.NoSuchWarehouseExceptioncorresponding to this error responseRationale
The
/v1/configendpoint allows an optional query parameter for a warehouse identifier, e.g./v1/config?warehouse=mywarehouse. But the openapi spec does not specify what should happen if the requested warehouse does not exist.We noticed that Snowflake Open Catalog already returns a 404 for non-existent warehouses:
{ "error": { "message": "Unable to find warehouse NONEXISTENT_WAREHOUSE_12345", "type": "NotFoundException", "code": 404 } }This PR therefore formalizes what Snowflake Open Catalog is already doing in production. It seems sensible to formalize the 404 response code, because this is consistent with other Iceberg REST endpoints which allow a 404 response code for missing respources (tables, namespaces, views).
Our use case
At my company we triage Iceberg exceptions according to their type. For example, an exception for a missing resource might get triaged to the team responsible for providing that resource.
Currently, the Iceberg core library yields the generic
RESTExceptionif a warehouse does not exist. It is difficult to automatically triage the genericRESTException. It would help us if Iceberg could yield theNoSuchWarehouseExceptioninstead, so our application code can more confidently triage this runtime issue.We expect other Iceberg users have similar operational workflows where distinguishing warehouse configuration errors from other failures would improve their incident response.
See the corresponding devlist thread at https://lists.apache.org/thread/xshfmp6m78l4ys6loozwop0fjl0cov3d