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: [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]
