-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core: introduce shared authentication refresh executor #12563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,10 @@ | |
| /** | ||
| * An {@link AuthManager} that provides machinery for refreshing authentication data asynchronously, | ||
| * using a background thread pool. | ||
| * | ||
| * @deprecated since 1.10.0, will be removed in 1.11.0; use {@link ThreadPools#authRefreshPool()}. | ||
| */ | ||
| @Deprecated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we're deprecating this class, then I don't think we should be removing functionality here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in fact I believe we should only be deprecating this here without updating the functionality (as that would be a breaking change). Once
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if somebody else from the community is extending
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What we can do though, is revert the changes to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I commented before I saw your comment 😅 – I agree that's an issue.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes that's what I was proposing in my earlier comment |
||
| public abstract class RefreshingAuthManager implements AuthManager { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(RefreshingAuthManager.class); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that we eventually want to share this thread pool, I wonder whether the default should be higher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, yeah ok I think 1 should be fine then