Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Mar 16, 2022

This removes the 404 response from the REST catalog when dropping a table. The Catalog API should not throw NoSuchTableException and instead returns false if the table did not exist. The equivalent behavior in the REST API is to always return 200 with false for the dropped field.

Related to this comment: #4322 (comment)

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

+1 LGTM. This matches better the existing spec.

Will we differentiate between NamespaceDoesNotExist and TableDoesNotExist in the client or does it not matter? The catalog interface sort of implies it doesn't matter.

@rdblue
Copy link
Contributor Author

rdblue commented Mar 16, 2022

Will we differentiate between NamespaceDoesNotExist and TableDoesNotExist in the client or does it not matter? The catalog interface sort of implies it doesn't matter.

It doesn't matter. If the table did not exist for any reason, it should return false.

@rdblue rdblue requested a review from danielcweeks March 17, 2022 00:03
@danielcweeks
Copy link
Contributor

I think there's a distinction between what what the REST implementation should do and what the client behavior should be. I'm not sure if the drop behavior is just extending from spark, but I think there's good reason that we should return the appropriate 404:

  1. Other endpoints return 404 for non-existent namespace/tables, which makes this behavior inconsistent
  2. Returning 200/false doesn't actually convey why it was not dropped, just that it was not dropped
  3. The client implementation can always handle the 404 and return false

@rdblue
Copy link
Contributor Author

rdblue commented Mar 20, 2022

@danielcweeks, thinking about this more, I agree with you. Looks like there's a fair amount of debate about this. Most people side with using 404 and I agree that the protocol doesn't necessarily need to go one way or another to have an idempotent API.

What made me change my mind in the end is that we can use 204 and 404 for true/false cases rather than having a response object. That makes all the implementations simpler.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants