You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/03/18 12:04:02 UTC
[PR] Core: introduce shared authentication refresh executor [iceberg]
adutra opened a new pull request, #12563:
URL: https://github.com/apache/iceberg/pull/12563
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-3023409215
FYI I rebased the PR since it's been a while.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177620296
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -53,6 +53,14 @@ private SystemConfigs() {}
Math.max(2, 4 * Runtime.getRuntime().availableProcessors()),
Integer::parseUnsignedInt);
+ /** Sets the core size of the thread pool used for refreshing authentication data. */
+ public static final ConfigEntry<Integer> AUTH_REFRESH_THREAD_POOL_SIZE =
+ new ConfigEntry<>(
+ "iceberg.rest.auth.refresh.num-threads",
+ "ICEBERG_AUTH_REFRESH_NUM_THREADS",
+ 1,
Review Comment:
ah right, yeah ok I think 1 should be fine then
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2175102382
##########
core/src/main/java/org/apache/iceberg/util/ThreadPools.java:
##########
@@ -82,6 +87,20 @@ public static ExecutorService getDeleteWorkerPool() {
return DELETE_WORKER_POOL;
}
+ /**
+ * A shared {@link ScheduledExecutorService} that REST catalogs can use for refreshing their
+ * authentication data.
+ */
+ public static ScheduledExecutorService getAuthRefreshPool() {
Review Comment:
```suggestion
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12563:
URL: https://github.com/apache/iceberg/pull/12563
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-2814971470
@nastra @danielcweeks can we consider this for 1.10 please?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-2921638232
Pinging @nastra and @danielcweeks again: do you think we can consider this one for 1.10?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-3017323760
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2175113613
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
Review Comment:
version info needs to be updated here, since the deprecation applies with 1.10.0 and we can only remove with 1.11.0
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
it's not clear to me why we're deprecating the entire class here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-2917892178
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2176566947
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177554442
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -62,6 +64,7 @@ public class OAuth2Manager extends RefreshingAuthManager {
private long startTimeMillis;
private OAuthTokenResponse authResponse;
private AuthSessionCache sessionCache;
+ private ScheduledExecutorService refreshExecutor;
Review Comment:
Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177618295
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -122,9 +123,8 @@ public static void afterClass() throws Exception {
// there aren't other token refreshes being scheduled after every sign request and after
// TestS3RestSigner completes all tests, there should be only this single token in the queue
// that is scheduled for refresh
- assertThat(validatingSigner.icebergSigner)
- .extracting("authManager")
- .extracting("refreshExecutor")
+ assertThat(ThreadPools.authRefreshPool())
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177150933
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
@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).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177343322
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -62,6 +64,7 @@ public class OAuth2Manager extends RefreshingAuthManager {
private long startTimeMillis;
private OAuthTokenResponse authResponse;
private AuthSessionCache sessionCache;
+ private ScheduledExecutorService refreshExecutor;
Review Comment:
Well, for me there wasn't any risk of breaking change here – but OK. Let me change this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177867730
##########
core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java:
##########
@@ -63,10 +63,6 @@ void initSessionNoOAuth2Properties() {
try (OAuth2Manager manager = new OAuth2Manager("test");
OAuth2Util.AuthSession session = manager.initSession(client, properties)) {
assertThat(session.headers()).isEmpty();
- assertThat(manager)
Review Comment:
see my other comment (https://github.com/apache/iceberg/pull/12563#discussion_r2177618295), so we should be rolling back the changes here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-3143332410
@nastra @danielcweeks can we try to get this in please? It was already approved.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-3141722682
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177180362
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177218787
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
What we can do though, is revert the changes to `RefreshingAuthManager` just in case someone is extending that class.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177323163
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -62,6 +64,7 @@ public class OAuth2Manager extends RefreshingAuthManager {
private long startTimeMillis;
private OAuthTokenResponse authResponse;
private AuthSessionCache sessionCache;
+ private ScheduledExecutorService refreshExecutor;
Review Comment:
I don't think we need any changes to this class. We would update it once `RefreshingAuthManager` has been removed right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-3019097762
@nastra @danielcweeks : ping for review, please.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2175115590
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
if we're deprecating this class, then I don't think we should be removing functionality here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#issuecomment-2814237715
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2175095051
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
it's not clear to me why we're deprecating the entire class here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177318483
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
> 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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177541356
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -53,6 +53,14 @@ private SystemConfigs() {}
Math.max(2, 4 * Runtime.getRuntime().availableProcessors()),
Integer::parseUnsignedInt);
+ /** Sets the core size of the thread pool used for refreshing authentication data. */
+ public static final ConfigEntry<Integer> AUTH_REFRESH_THREAD_POOL_SIZE =
+ new ConfigEntry<>(
+ "iceberg.rest.auth.refresh.num-threads",
+ "ICEBERG_AUTH_REFRESH_NUM_THREADS",
+ 1,
Review Comment:
given that we eventually want to share this thread pool, I wonder whether the default should be higher
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177563375
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -53,6 +53,14 @@ private SystemConfigs() {}
Math.max(2, 4 * Runtime.getRuntime().availableProcessors()),
Integer::parseUnsignedInt);
+ /** Sets the core size of the thread pool used for refreshing authentication data. */
+ public static final ConfigEntry<Integer> AUTH_REFRESH_THREAD_POOL_SIZE =
+ new ConfigEntry<>(
+ "iceberg.rest.auth.refresh.num-threads",
+ "ICEBERG_AUTH_REFRESH_NUM_THREADS",
+ 1,
Review Comment:
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?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: introduce shared authentication refresh executor [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #12563:
URL: https://github.com/apache/iceberg/pull/12563#discussion_r2177261686
##########
core/src/main/java/org/apache/iceberg/rest/auth/RefreshingAuthManager.java:
##########
@@ -18,73 +18,27 @@
*/
package org.apache.iceberg.rest.auth;
-import java.util.List;
-import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.iceberg.util.ThreadPools;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
- * 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()}.
*/
+@Deprecated
Review Comment:
Sorry I commented before I saw your comment 😅 – I agree that's an issue.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org