Core/REST: generify AuthSessionCache #12562

adutra · 2025-03-18T11:51:28Z

Currently, AuthSessionCache is not generic and only accepts keys of type String.

This wasn't a good design choice. A common pattern I've seen already is to create cache entries where the key is some arbitrary configuration object.

To allow this, we need to be able to create generic caches e.g.new AuthSessionCache<MyAuthConfig, MyAuthSession>. This PR enables that.

Unfortunately that requires some revapi exceptions. I think they are acceptable because it's unlikely that anybody – apart from me :-) – is using AuthSessionCache; but I could also introduce a second AuthSessionCache instead and deprecate the existing one, if that's better.

github-actions · 2025-04-18T00:16:21Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

adutra · 2025-04-18T09:01:19Z

@nastra @danielcweeks can we consider this for 1.10 please?

github-actions · 2025-05-29T00:17:37Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

adutra · 2025-05-30T08:43:47Z

Pinging @nastra and @danielcweeks again: do you think we can consider this one for 1.10?

github-actions · 2025-06-30T00:19:30Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

dimas-b · 2025-06-30T14:21:36Z

@nastra @danielcweeks : please review :)

nastra · 2025-07-01T06:54:47Z

.palantir/revapi.yml

      new: "class org.apache.iceberg.Metrics"
      justification: "Java serialization across versions is not guaranteed"
    org.apache.iceberg:iceberg-core:
+    - code: "java.generics.elementNowParameterized"


I don't think we should be breaking the API. Instead it's probably best to introduce a new class and deprecate the old one.
Do we actually have use cases that require a parameterized cache? If so, could you please link to some examples in order for me to better understand why we're doing these changes (in the description you mentioned that you're seeing a common pattern, but it would be good to share where those patterns are used)

I don't think we should be breaking the API. Instead it's probably best to introduce a new class and deprecate the old one.

Yes I'm OK with that. I will update the PR.

Do we actually have use cases that require a parameterized cache? If so, could you please link to some examples in order for me to better understand why we're doing these changes (in the description you mentioned that you're seeing a common pattern, but it would be good to share where those patterns are used)

Yes, the Dremio Auth Manager has a different caching pattern:

https://github.com/dremio/iceberg-auth-manager/blob/93517cb30700507f2be5c5688508ca9d0d081c76/oauth2/core/src/main/java/com/dremio/iceberg/authmgr/oauth2/cache/AuthSessionCache.java

The caching pattern here is imo a fairly natural one: we use a configuration class as the key, so that every time the same configuration comes up again, we reuse the cached session.

Currently, `AuthSessionCache` is not generic and only accepts keys of type String. This wasn't a good design choice. A common pattern I've seen already is to create cache entries where the key is some arbitrary configuration object. To allow this, we need to be able to create generic caches e.g.`new AuthSessionCache<MyAuthConfig, MyAuthSession>`. This PR enables that. Unfortunately that requires some revapi exceptions. I think they are acceptable, but I could also introduce a second `AuthSessionCache` instead and deprecate the existing one.

adutra · 2025-07-01T12:58:16Z

I don't think we should be breaking the API. Instead it's probably best to introduce a new class and deprecate the old one.

Done. FYI I rebased.

adutra · 2025-07-01T13:00:21Z

core/src/main/java/org/apache/iceberg/rest/auth/GenericAuthSessionCache.java

+import org.slf4j.LoggerFactory;
+
+/** A cache for {@link AuthSession} instances. */
+public class GenericAuthSessionCache<K, V extends AuthSession> implements AutoCloseable {


@nastra I wonder if it's worth extracting an interface, wdyt? Just let me know.

what would be the advantage of having an interface in this case? Also I would probably just name this SessionCache

danielcweeks · 2025-07-01T18:23:22Z

Unfortunately that requires some revapi exceptions. I think they are acceptable because it's unlikely that anybody – apart from me :-) – is using AuthSessionCache; but I could also introduce a second AuthSessionCache instead and deprecate the existing one, if that's better.

@adutra Unfortunately, our revapi requirements are more strict for core and we can't just assume other people haven't built against it. We need to go through deprecation cycle, so this isn't acceptable without maintaining the existing apis/functionality.

adutra · 2025-07-01T18:52:16Z

Unfortunately, our revapi requirements are more strict for core and we can't just assume other people haven't built against it. We need to go through deprecation cycle, so this isn't acceptable without maintaining the existing apis/functionality.

Hi @danielcweeks yes, @nastra told me the same thing and I updated the PR accordingly. There are no revapi exceptions anymore.

nastra · 2025-07-02T09:55:35Z

core/src/main/java/org/apache/iceberg/rest/auth/GenericAuthSessionCache.java

+  public GenericAuthSessionCache(String name, Duration sessionTimeout) {
+    this(
+        sessionTimeout,
+        ThreadPools.newExitingWorkerPool(name + "-auth-session-evict", 1),


should this be using the new auth pool once it's merged?

@danielcweeks suggested a dedicated pool here:

#11844 (comment)

I'm fine either way.

nastra · 2025-07-02T09:59:28Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

  }

+  /**
+   * @deprecated since 1.10.0, will be removed in 1.11.0.


what's the alternative impl that people should be switching to? I think part of this PR we should be adding a new method that returns the generic session cache and switch sessionCache in this class to use it

nastra · 2025-07-03T09:23:03Z

core/src/main/java/org/apache/iceberg/rest/auth/AuthManagerSessionCache.java

+import java.util.function.Function;
+
+/** A cache for {@link AuthSession} instances. */
+public interface AuthManagerSessionCache<K, V extends AuthSession> extends AutoCloseable {


is that interface really needed to achieve what we want to achieve here?

Yes, because I need to wrap the old deprecated AuthSessionCache in an adapter that bridges the old class to the new interface.

nastra · 2025-07-03T09:24:24Z

core/src/main/java/org/apache/iceberg/rest/auth/DefaultAuthManagerSessionCache.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DefaultAuthManagerSessionCache<K, V extends AuthSession>


my personal take is that the name is quite long and I would prefer just SessionCache. The package name includes auth so that makes it clearer to me that this is an auth session cache

nastra · 2025-07-03T09:28:16Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

+   */
+  @Deprecated
  protected AuthSessionCache newSessionCache(String managerName, Map<String, String> properties) {
-    return new AuthSessionCache(managerName, sessionTimeout(properties));


we can't return null here as that would be a behavioral change. We can only deprecate and leave the impl as-is. Also I don't think we need the AuthManagerSessionCacheAdapter. We would basically keep newSessionCache as-is and then have newAuthSessionCache that returns the parameterized version of the cache

I'm not sure I agree. The point here is: what to do if someone extended this class and overrode the newSessionCache method?

If they did that, it's because they have a custom cache, and we need to honor their intent and thus use the custom cache + adapter.

However, if the method is not overridden, then they were using the default cache, and in this case we want to use the new, generic cache instead.

Returning null here allows to distinguish both situations. It's thus not a behavioral change.

What if people didn't override newSessionCache and just upgrade Iceberg and realize that this method now returns null? Not everyone will be using newAuthSessionCache immediately

@adutra if you revert the last commit and apply the below diff, wouldn't that work and avoid all of the extra complexity?

--- a/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java +++ b/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java @@ -61,7 +61,7 @@ public class OAuth2Manager extends RefreshingAuthManager { private RESTClient refreshClient; private long startTimeMillis; private OAuthTokenResponse authResponse; - private AuthSessionCache sessionCache; + private SessionCache<String, OAuth2Util.AuthSession> sessionCache; public OAuth2Manager(String managerName) { super(managerName + "-token-refresh"); @@ -105,7 +105,7 @@ public class OAuth2Manager extends RefreshingAuthManager { RESTClient sharedClient, Map<String, String> properties) { // This client will be used for token refreshes; it should not have an auth session. this.refreshClient = sharedClient.withAuthSession(AuthSession.EMPTY); - this.sessionCache = newSessionCache(name, properties); + this.sessionCache = newAuthSessionCache(name, properties); AuthConfig config = AuthConfig.fromProperties(properties); Map<String, String> headers = OAuth2Util.authHeaders(config.token()); OAuth2Util.AuthSession session = new OAuth2Util.AuthSession(headers, config); @@ -159,7 +159,7 @@ public class OAuth2Manager extends RefreshingAuthManager { try { super.close(); } finally { - AuthSessionCache cache = sessionCache; + SessionCache<String, OAuth2Util.AuthSession> cache = sessionCache; this.sessionCache = null; if (cache != null) { cache.close(); @@ -175,6 +175,11 @@ public class OAuth2Manager extends RefreshingAuthManager { return new AuthSessionCache(managerName, sessionTimeout(properties)); } + protected SessionCache<String, OAuth2Util.AuthSession> newAuthSessionCache( + String managerName, Map<String, String> properties) { + return new SessionCache<>(managerName, sessionTimeout(properties)); + } +

It would, but if someone is overriding the deprecated method, the overridden impl would be ignored. But I'm fine with that.

nastra · 2025-07-03T13:33:57Z

core/src/main/java/org/apache/iceberg/rest/auth/SessionCache.java

+import java.util.function.Function;
+
+/** A cache for {@link AuthSession} instances. */
+public interface SessionCache<K, V extends AuthSession> extends AutoCloseable {


can you please elaborate why we still need a separate interface? I'd like to understand the use case you're trying to solve here. Currently I don't see any need for having a separate interface and DefaultSessionCache could be named just SessionCache. Also DefaultSessionCache already is parameterized, so that should give us enough flexibility?

An interface could allow people to use another cache library than Caffeine, or create the Caffeine cache in a different way. Generally speaking an interface is more flexible for implementors than a superclass.

Also, as @danielcweeks pointed out, the default implementation could be made package-private.

But if you insist on removing the interface, I won't oppose.

I'm not insisting but rather trying to understand the actual use case and whether there's a need to add this kind of flexibility. Let's also see what @danielcweeks thinks about this

@nastra and @danielcweeks which direction should I move in?

Keep the SessionCache interface and change the visibility of DefaultSessionCache to package-private

Remove the interface and rename DefaultSessionCache to SessionCache

danielcweeks · 2025-07-03T14:49:08Z

core/src/main/java/org/apache/iceberg/rest/auth/DefaultAuthManagerSessionCache.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DefaultAuthManagerSessionCache<K, V extends AuthSession>


This doesn't look like it needs to be public.

danielcweeks · 2025-07-03T15:16:03Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

  private long startTimeMillis;
  private OAuthTokenResponse authResponse;
-  private AuthSessionCache sessionCache;
+  private AuthManagerSessionCache<String, OAuth2Util.AuthSession> sessionCache;


Don't you want this to be AuthManagerSessionCache<?, OAuth2Util.AuthSession>? I don't think we need to preserve the string generic or maybe I'm misunderstanding what the end state is. If we don't have the new protected newAuthSessionCache return a wider generic, we'll have to go through another deprecation cycle.

The idea was to allow other manager impls to use a different cache key. But this auth manager should keep using string keys imho. It's not the intent of this PR to change that.

danielcweeks · 2025-07-08T16:23:44Z

@adutra I'm not sure I fully understand the value of this change at this point. We're making a breaking change that's going to take multiple releases to finalize, but I'm not convinced that there's real value added here.

Maybe you can add more context on how you want to use this, but if you're going to key off an object, you need to have that object to retrieve the value. If you have the object, you can produce and identifier string for the key. What's the practical difference between:

AuthSessionCache<MyAuthConfig, MyAuthSession>, where MyAuthConfig is the key
new AuthSessionCache<String, MyAuthSession>, where the String is MyAuthConfig::sessionId?

If this is targeted at other implementations, they can easily hide this and you avoid the problems of using objects ask keys (e.g. properly implementing hashing/equivalence). I don't see a lot of upsides to geneaicizing this and there are number of downsides.

adutra · 2025-07-09T11:15:04Z

If you have the object, you can produce and identifier string for the key.

Not always. There is no "session ID" in my use case. You could obviously use the object's hash, but how do you handle hash collisions? You could also serialize the object to JSON or some other format, but that would be very inefficient.

you avoid the problems of using objects ask keys (e.g. properly implementing hashing/equivalence)

We did take care of implementing fast equals and hashCode.

github-actions · 2025-08-09T00:17:42Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2025-08-17T00:20:07Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added the core label Mar 18, 2025

github-actions bot added the stale label Apr 18, 2025

adutra force-pushed the generic-session-cache branch from 27d51c8 to 8f60273 Compare April 18, 2025 09:00

github-actions bot removed the stale label Apr 19, 2025

danielcweeks self-requested a review April 28, 2025 15:28

github-actions bot added the stale label May 29, 2025

github-actions bot removed the stale label May 31, 2025

github-actions bot added the stale label Jun 30, 2025

github-actions bot removed the stale label Jul 1, 2025

nastra reviewed Jul 1, 2025

View reviewed changes

adutra added 2 commits July 1, 2025 14:54

review

8c61b3c

adutra force-pushed the generic-session-cache branch from 8f60273 to 8c61b3c Compare July 1, 2025 12:57

adutra commented Jul 1, 2025

View reviewed changes

nastra reviewed Jul 2, 2025

View reviewed changes

extract interface

b6ca1ef

nastra reviewed Jul 3, 2025

View reviewed changes

review

fafe528

nastra reviewed Jul 3, 2025

View reviewed changes

danielcweeks reviewed Jul 3, 2025

View reviewed changes

github-actions bot added the stale label Aug 9, 2025

github-actions bot closed this Aug 17, 2025

Core/REST: generify AuthSessionCache #12562

Core/REST: generify AuthSessionCache #12562

Uh oh!

Conversation

adutra commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 18, 2025

Uh oh!

adutra commented Apr 18, 2025

Uh oh!

github-actions bot commented May 29, 2025

Uh oh!

adutra commented May 30, 2025

Uh oh!

github-actions bot commented Jun 30, 2025

Uh oh!

dimas-b commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra commented Jul 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks commented Jul 1, 2025

Uh oh!

adutra commented Jul 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks commented Jul 8, 2025

Uh oh!

adutra commented Jul 9, 2025

Uh oh!

github-actions bot commented Aug 9, 2025

Uh oh!

github-actions bot commented Aug 17, 2025

Uh oh!

Reviewers

adutra commented Mar 18, 2025 •

edited

Loading

danielcweeks Jul 3, 2025 •

edited

Loading