Correctly Propagate Errors from Drop Entities#3693
Open
travis-bowen wants to merge 2 commits intoapache:mainfrom
Open
Correctly Propagate Errors from Drop Entities#3693travis-bowen wants to merge 2 commits intoapache:mainfrom
travis-bowen wants to merge 2 commits intoapache:mainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During drop commands, Apache Polaris would make assumptions based on the entity type as to why the entity could not be dropped which could lead to incorrect user error messaging and handling.
Here I tackle updating error handling for DropTable, DropNamespace, and DropView.
The Catalog API javadocs by Iceberg indicate false should indicate [TableNotExists] (and simila for View) (https://github.com/apache/iceberg/blob/a8ece055ba93adc0c046db29b6b7b5edaf35d4da/api/src/main/java/org/apache/iceberg/catalog/Catalog.java#L292).
For Namespaces it's less clear, but I assume the same holds and we treat it to follow the same logic that false = Not Found in CatalogHandlerUtils.
Before
Any non success in Polaris's IcebergCatalog impl would return false.
Polaris's CatalogHandlerUtils would interpret this false to mean the entity did not exist and would throw the appropriate does not exist error
However, the reason that it returned false, may not truly be that it didn't exist.
After
The IcebergCatalog impl in Polaris uses the return status of the persistence result to determine whether or not the drop failure was due to being not found. If not found, it returns false per the interface definition, which the CatalogHandlerUtil transforms into the appropriate not found exception. Otherwise it throws it's own error corresponding to the cause.
Ran tests locally + S3 Tests with drop


Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)