Auth Manager API part 2: AuthManager#11809
Conversation
| * when the session is no longer needed. Implementations should override {@link #close()} to release | ||
| * any resources. | ||
| */ | ||
| public interface AuthSession extends AutoCloseable { |
There was a problem hiding this comment.
The naming issue from the previous PR is still pending. @nastra and myself agreed recently on HTTPAuthSession – @danielcweeks is that OK with you?
There was a problem hiding this comment.
I talked with @danielcweeks and we probably want to go with what you have here as we should eventually deprecate the AuthSession in OAuth2Util and so we shouldn't have an issue with conflicting names for too long
| * <p>It is not required to cache the returned session internally, as the catalog will keep it | ||
| * alive for the lifetime of the catalog. | ||
| */ | ||
| AuthSession mainSession(RESTClient sharedClient, Map<String, String> properties); |
There was a problem hiding this comment.
I feel like catalogSession makes more sense here instead of mainSession
There was a problem hiding this comment.
Ah sorry I forgot to start this discussion :-)
But wdyt about components like the signer and the credential refreshers? They will also use the auth manager. But in those places, it doesn't make sense to talk about a "catalog" session. WDYT?
There was a problem hiding this comment.
I'm not too concerned about using catalogSession in other places because it actually reflects the session provided to the catalog.
There was a problem hiding this comment.
Reverted to catalogSession.
| * <p>This method is called when the owning catalog is closed. | ||
| */ | ||
| @Override | ||
| default void close() { |
There was a problem hiding this comment.
is there any value in having a default impl that doesn't do anything? I feel like this should be just left to the actual auth manager implementations
|
|
||
| @Test | ||
| void authenticate() { | ||
|
|
There was a problem hiding this comment.
nit: unnecessary newline here and in the other test methods in this class
| private AuthManagers() {} | ||
|
|
||
| public static AuthManager loadAuthManager(String name, Map<String, String> properties) { | ||
|
|
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM pending the two points around renames
| DynConstructors.builder(AuthManager.class) | ||
| .loader(AuthManagers.class.getClassLoader()) | ||
| .impl(impl, String.class) // with name | ||
| .impl(impl) // without name |
There was a problem hiding this comment.
This seems unnecessary. I couldn't find other places where we try for multiple constructors. I think we lean toward one convention (though I may have missed examples).
There was a problem hiding this comment.
I took this idea from org.apache.iceberg.LocationProviders. I found it interesting because the name may be of relevance for some impls (e.g. the Oauth2 manager will use it to name the token refresh executor threads) but will be useless for more simple impls, and forcing those impls to declare a constructor with an unused parameter seemed overkill.
There was a problem hiding this comment.
There are specific tests for this feature in org.apache.iceberg.TestLocationProvider.
There was a problem hiding this comment.
Do we use the name anywhere? Seems like if we use it, we should require it. If we don't use it, we should just get rid of it. Overall, I prefer having an opinionated path as opposed to providing lots of options.
There was a problem hiding this comment.
Okay I kept only the constructor with name.
|
Couple minor things. I think we should revert to the original names (per the comments) |
| * the cache, or the cache itself is closed. | ||
| */ | ||
| @Override | ||
| void close(); |
There was a problem hiding this comment.
@nastra per your previous comment, I thought you would prefer this method to be non-default as well.
3 participants
Second PR for the Auth Manager API.
This PR introduces the
AuthManageritself, along with 2 simple implementations: Basic and Noop. This PR does not turn the manager on yet.\cc @nastra @danielcweeks