oguzhanunlu commented on code in PR #15746:
URL: https://github.com/apache/iceberg/pull/15746#discussion_r2988198042
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -162,6 +162,8 @@ paths:
$ref: '#/components/responses/UnauthorizedResponse'
403:
$ref: '#/components/responses/ForbiddenResponse'
+ 404:
+ $ref: '#/components/responses/NoSuchWarehouseResponse'
Review Comment:
Thank you both for the feedback, let me share my view on both points.
@kevinjqliu That is an example from the Snowflake payload and I'd prefer
`NotFoundResponse` to align with naming convention in the spec. However, I
realized that 404 responses are inlined in all endpoints and named responses
are used for status codes that mean the same thing across endpoints e.g. 400,
401, 403. It makes sense because what's `not found` is endpoint-dependent
(namespace, table, view etc.) hence I think `/v1/config` 404 response should be
inlined as well instead of adding e.g. `NotFoundResponse`. Would you agree?
@rambleraptor , @danielcweeks
[raised](https://github.com/apache/iceberg/pull/14965#issuecomment-3747136540)
a valid concern in the original PR #14965: 404 from config endpoint is
ambiguous. It could mean "warehouse not found" or "misconfigured URL pointing
to a non-Iceberg server". If clients only check HTTP status code, they can't
distinguish the two. How would you suggest handling that? e.g. PlanErrorHandler
checks error type to decide which exception to throw.
Looking forward to hearing what you think.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
