Skip to content

Conversation

@MChamberlin
Copy link

@MChamberlin MChamberlin commented Jan 15, 2025

Some REST catalog implementations (including the Snowflake Polaris catalog) return a 401 when a token is expired, raising an UnauthorizedError within pyiceberg. Refreshing the token, as is done for 419 responses, resolves the issue.

This commit adds UnauthorizedError to the retry exception types for REST catalog calls, such that the token will be refreshed and the call retried.

@kevinjqliu
Copy link
Contributor

Hey @MChamberlin Thanks for the PR.

I have some concerns related to this approach

Some REST catalog implementations (including the Snowflake Polaris catalog) return a 401 when a token is expired, raising an UnauthorizedError within pyiceberg.

401 status is specifically for unauthorized requests https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401
If we add 401 to the list of retry-able errors, we'd have to retry for other "truly" unauthorized errors which does not feel like the right thing to do.

My suggestion would be to change the catalog implementation to send 419 instead. The REST spec specifically calls out 401 as UnauthorizedResponse and 419 as AuthenticationTimeoutResponse

@sungwy
Copy link
Collaborator

sungwy commented Jan 16, 2025

I agree with @kevinjqliu here. I've created this issue on polaris github to fix the root cause on the catalog instead: apache/polaris#791

@MChamberlin
Copy link
Author

My suggestion would be to change the catalog implementation to send 419 instead. The REST spec specifically calls out 401 as UnauthorizedResponse and 419 as AuthenticationTimeoutResponse

Good point, I was thinking about this in the context of the Snowflake Polaris service, which I can't say for sure uses the OSS implementation. But the problem of course is upstream in the catalog responses either way. I will close the PR.

@MChamberlin MChamberlin reopened this Mar 8, 2025
@MChamberlin
Copy link
Author

@kevinjqliu @sungwy Given the decision on apache/polaris#791, I am re-opening this PR.

@sungwy
Copy link
Collaborator

sungwy commented Mar 8, 2025

Hi @MChamberlin thanks for following up! This PR was recently merged to introduce the fix following the decision to deprecate the 419 status code in the catalog community. Could you let you us know if the latest version of the code in main works for you as expected? #1741

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