[GitHub] [iceberg] danielcweeks commented on pull request #4830: Core: Add session catalog implementation for REST

[GitBox <gi...@apache.org>]

danielcweeks commented on PR #4830:
URL: https://github.com/apache/iceberg/pull/4830#issuecomment-1133472880

   Overall, the logic and sequencing looks sound.  The one thing I might suggest is actually creating a separate package `org...iceberg.rest.auth` and put much of the auth code there.  RESTSessionCatalog class not has a lot of internal logic that's really about OAuth2 which could be pulled out separately (e.g. `AuthSession` and the static methods supporting flows).  Some minor refactoring could help in terms of adding other auth in the future.


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org