danielcweeks commented on PR #15703: URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4346383173
A few high-level observations: 1. I think we should keep all the implementation in the same package and restrict everything to package private where possible. It's not worth losing the encapsulation to add a little bit if package structure. 2. We're leaking many of the Nimbus primitives through the interfaces. If we can keep those within the package, I'm probably ok with that, but if we expose them through public interfaces, it binds us to that implementation. 3. We use `Optional` and optional fields very inconsistently. If it's truly optional, then it should be wrapped in `Optional`. 4. I'm not convinced we should be breaking out separate configs like `BasicConfig`, `TokenRefreshConfig` into composed separate sets of interfaces. I might be able to get behind keeping refresh and exchange configs separate, but it feels like a lot of levels of indirection. -- 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]
