Core: introduce shared authentication refresh executor by adutra · Pull Request #12563 · apache/iceberg

adutra · 2025-03-18T12:03:59Z

Currently, each AuthManager instance manages its own refresh pool. During the lifecycle of the catalog session, a few AuthManager instances may be created: one for the catalog itself, and a few more in components like the S3 signer client, and credential refreshers. Each one of them is currently creating their own pool, which is a waste of resources.

This PR introduces a shared refresh pool for use by AuthManagers, thus allowing to minimize the amount of threads allocated for auth session refreshes. The shared pool is closed by a shutdown hook.

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

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 dev@iceberg.apache.org list. Thank you for your contributions.

adutra · 2025-04-18T09:03:13Z

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

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

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 dev@iceberg.apache.org list. Thank you for your contributions.

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

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

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

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 dev@iceberg.apache.org list. Thank you for your contributions.

dimas-b · 2025-06-30T13:10:47Z

@nastra @danielcweeks : ping for review, please.

nastra · 2025-06-30T13:36:35Z

+   * A shared {@link ScheduledExecutorService} that REST catalogs can use for refreshing their
+   * authentication data.
+   */
+  public static ScheduledExecutorService getAuthRefreshPool() {


Suggested change

public static ScheduledExecutorService getAuthRefreshPool() {

public static ScheduledExecutorService authRefreshPool() {

I'm aware that we already have some methods in this class with a get prefix, but those methods have been added at the very beginning and haven't been renamed

nastra · 2025-06-30T13:41:19Z

 /**
- * An {@link AuthManager} that provides machinery for refreshing authentication data asynchronously,
- * using a background thread pool.
+ * @deprecated since 1.9.0, will be removed in 1.10.0; use {@link ThreadPools#getAuthRefreshPool()}.


version info needs to be updated here, since the deprecation applies with 1.10.0 and we can only remove with 1.11.0

nastra · 2025-06-30T13:42:16Z

- * using a background thread pool.
+ * @deprecated since 1.9.0, will be removed in 1.10.0; use {@link ThreadPools#getAuthRefreshPool()}.
 */
+@Deprecated


if we're deprecating this class, then I don't think we should be removing functionality here?

in fact I believe we should only be deprecating this here without updating the functionality (as that would be a breaking change). Once RefreshingAuthManager is removed, we can switch the OAuth2Manager to use the new auth refresh pool, but I don't think we can deprecate + switch within the same release

@nastra I don't mind doing the change in 2 steps, but in which case do you think this would be a breaking change? The API is not affected (and revapi didn't complain).

what if somebody else from the community is extending RefreshingAuthManager and relying on its functionality? We're effectively removing the "refresh" part, which is a breaking change in behavior, even though RevAPI wasn't complaining

What we can do though, is revert the changes to RefreshingAuthManager just in case someone is extending that class.

Sorry I commented before I saw your comment 😅 – I agree that's an issue.

What we can do though, is revert the changes to RefreshingAuthManager just in case someone is extending that class.

Yes that's what I was proposing in my earlier comment

Currently, each `AuthManager` instance manages its own refresh pool. During the lifecycle of the catalog session, a few `AuthManager` instances may be created: one for the catalog itself, and a few more in components like the S3 signer client, and credential refreshers. Each one of them is currently creating their own pool, which is a waste of resources. This PR introduces a shared refresh pool for use by `AuthManager`s, thus allowing to minimize the amount of threads allocated for auth session refreshes. The shared pool is closed by a shutdown hook.

adutra · 2025-07-01T11:00:23Z

FYI I rebased the PR since it's been a while.

nastra · 2025-07-01T11:23:12Z

  private long startTimeMillis;
  private OAuthTokenResponse authResponse;
  private AuthSessionCache sessionCache;
+  private ScheduledExecutorService refreshExecutor;


I don't think we need any changes to this class. We would update it once RefreshingAuthManager has been removed right?

Well, for me there wasn't any risk of breaking change here – but OK. Let me change this.

nastra · 2025-07-01T12:57:43Z

+      new ConfigEntry<>(
+          "iceberg.rest.auth.refresh.num-threads",
+          "ICEBERG_AUTH_REFRESH_NUM_THREADS",
+          1,


given that we eventually want to share this thread pool, I wonder whether the default should be higher

I think 1 is fine. Most of the time, we'll have only 1 auth manager per JVM, and sometimes 2 (e.g. request signing + credential refreshing) – but realistically, I think we won't see more than 2 per JVM. Given that the typical usage of this executor is to schedule token refreshes every hour or so, 1 is probably fine (also: this is the core pool size, not the maximum pool size). Wdyt?

ah right, yeah ok I think 1 should be fine then

nastra · 2025-07-01T13:31:20Z

-    assertThat(validatingSigner.icebergSigner)
-        .extracting("authManager")
-        .extracting("refreshExecutor")
+    assertThat(ThreadPools.authRefreshPool())


I think we should roll back these test changes here and in the other test class, since nothing really changed in terms of the implementation

nastra · 2025-07-01T15:12:25Z

    try (OAuth2Manager manager = new OAuth2Manager("test");
        OAuth2Util.AuthSession session = manager.initSession(client, properties)) {
      assertThat(session.headers()).isEmpty();
-      assertThat(manager)


see my other comment (#12563 (comment)), so we should be rolling back the changes here

nastra

LGTM, I'll also wait a bit in case @danielcweeks wants to review this

github-actions · 2025-08-01T00:21:59Z

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 dev@iceberg.apache.org list. Thank you for your contributions.

adutra · 2025-08-01T07:21:32Z

@nastra @danielcweeks can we try to get this in please? It was already approved.

github-actions Bot added core AWS labels Mar 18, 2025

adutra mentioned this pull request Mar 28, 2025

AuthManager: implementation for OAuth2 dremio/iceberg-auth-manager#1

Closed

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

adutra force-pushed the shared-auth-executor branch from 69ab7f4 to 9587a2e Compare April 18, 2025 09:02

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

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

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

nastra reviewed Jun 30, 2025

View reviewed changes

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

adutra added 2 commits July 1, 2025 12:53

review

6f256de

adutra force-pushed the shared-auth-executor branch from 9587a2e to 6f256de Compare July 1, 2025 11:00

nastra reviewed Jul 1, 2025

View reviewed changes

review

2cd78ba

nastra reviewed Jul 1, 2025

View reviewed changes

fix failing test

f20be10

github-actions Bot removed the AWS label Jul 1, 2025

nastra reviewed Jul 1, 2025

View reviewed changes

review

0da07ea

nastra approved these changes Jul 1, 2025

View reviewed changes

github-actions Bot added the stale label Aug 1, 2025

github-actions Bot removed the stale label Aug 2, 2025

nastra merged commit 76b2f87 into apache:main Aug 4, 2025
42 checks passed

adutra mentioned this pull request Aug 4, 2025

Adapt AuthManager to Iceberg 1.10 dremio/iceberg-auth-manager#127

Closed

	public static ScheduledExecutorService getAuthRefreshPool() {
	public static ScheduledExecutorService authRefreshPool() {

Uh oh!

Conversation

adutra commented Mar 18, 2025

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!

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!

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!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 1, 2025

Uh oh!

adutra commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants