sakshichitnis27 commented on PR #18665: URL: https://github.com/apache/iceberg/pull/18665#issuecomment-6635563751
Thanks for the review, @nastra. I pushed 157a292c4 with all three changes: - `RESTViewBuilder.replace()` now loads the view first and only checks for a same-named table when the view doesn't exist. Both probes go through one helper that treats a 401 or 403 as "no conflict", since the server still rejects a conflicting create. - `NotAuthorizedException` is now caught alongside `ForbiddenException`. - The tests moved to `TestRESTViewCatalog` and are parameterized over 401 and 403: replace and create-or-replace of existing and missing tables with view requests denied, and replace of existing and missing views with table requests denied. `TestRESTCatalog` is back to its original state. One behavior change to flag: when the server denies the view load, replacing a view over a name that is a table now surfaces `ForbiddenException` instead of `AlreadyExistsException`, because the load happens before the table check. A question on scope: #18662 also asks for `view-endpoints-supported=false` to be honored when the server advertises endpoints, and this PR doesn't change that. Should I change `Fixes #18662` to `Related to #18662` and handle that separately, or would you like it covered here? CI is green on the new commit. @sungwy, thanks for raising the probe question. nastra's point about keeping `AlreadyExistsException` uniform across catalogs makes sense to me, so the check stays. -- 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]
