Skip to content

API, Core: Handle 404 from /v1/config for missing warehouses#16059

Open
oguzhanunlu wants to merge 2 commits intoapache:mainfrom
oguzhanunlu:core/config-endpoint-404-handling
Open

API, Core: Handle 404 from /v1/config for missing warehouses#16059
oguzhanunlu wants to merge 2 commits intoapache:mainfrom
oguzhanunlu:core/config-endpoint-404-handling

Conversation

@oguzhanunlu
Copy link
Copy Markdown
Contributor

Summary

Add NoSuchWarehouseException and ConfigErrorHandler to handle 404 responses from the /v1/config endpoint when a configured warehouse does not exist.

This is the implementation counterpart to the spec change merged in #15746.

Handling ambiguous 404s

As discussed in #14965 , a 404 on /v1/config could mean either a missing warehouse or a misconfigured URL pointing to a non-Iceberg server. The handler checks error.type() != null to distinguish the two cases: valid Iceberg error responses include a type field, a required field per schema #/components/schemas/ErrorModel, while non-Iceberg 404s don't.

Add NoSuchWarehouseException and configErrorHandler that throws it on
404 responses with a valid error type, distinguishing missing warehouses
from misconfigured URIs. Update RESTSessionCatalog to use the new
handler for config calls.
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM one nit comment about where to place the tests

}
}

@Test
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit these tests look out of place, perhaps we can move them to core/src/test/java/org/apache/iceberg/rest/TestErrorHandlers.java

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kevinjqliu ! I just pushed a new commit to move them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants