Allow StsCredentialsProvider builder to take a ScheduledExecutorService #3260
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ecutorService for more efficient background processing, in the case where there are large numbers of credentials providers (e.g. a multi-tenant setup).
|
Nice contribution! What if the SDK by default shared threads between NonBlocking instances so that you don't have to configure it yourself? Is there a specific reason you want a custom thread pool? |
|
No specific reason for custom thread pool, other than perhaps configuring
the size based on demand, which could also be done via SDK configuration
(where the SDK was managing a shared pool).
It was the most minimal code change (pass in pool vs creating it); a more
integrated SDK managed one would be great.
Chris.
…On Wed, Jun 29, 2022 at 9:00 AM Matthew Miller ***@***.***> wrote:
Nice contribution! What if the SDK by default shared threads between
NonBlocking instances so that you don't have to configure it yourself? Is
there a specific reason you want a custom thread pool?
—
Reply to this email directly, view it on GitHub
<#3260 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR5RI743ZYSK3HQDL5NW45DVRRXI7ANCNFSM5ZNCHWXA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Let me take a swing at doing it within the SDK. The main challenge is scheduled executors only working with core threads, and core threads not being reused (even if idle), but we can probably work around that with a couple of executors or something more clever. It would be nice if everyone got this improvement, not just people 'in the know'. |
|
Cool. I was working around scheduled executor limitations by providing a
larger number of core threads (as it won’t increase pool size).
In looking at the two tiers of timers - scheduled executor task running
every minute that polls cred cache to see if it’s expired - perhaps a
timing wheel or other construct would be more appropriate to handle the
actual timer value.
…On Wed, Jun 29, 2022 at 9:08 AM Matthew Miller ***@***.***> wrote:
Let me take a swing at doing it within the SDK. The main challenge is
scheduled executors only working with core threads, and core threads not
being reused (even if idle), but we can probably work around that with a
couple of executors or something more clever.
It would be nice if everyone got this improvement, not just people 'in the
know'.
—
Reply to this email directly, view it on GitHub
<#3260 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR5RI7ZLMXSNKOEUBUOF4Z3VRRYJPANCNFSM5ZNCHWXA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Also related: to avoid situations where prefetching of credentials collude
(many tenants refreshing at same time and possibly saturating thread pool),
added a jitter (random second in x minute window) to prefetch time.
On Wed, Jun 29, 2022 at 9:13 AM Chris Lee ***@***.***>
wrote:
… Cool. I was working around scheduled executor limitations by providing a
larger number of core threads (as it won’t increase pool size).
In looking at the two tiers of timers - scheduled executor task running
every minute that polls cred cache to see if it’s expired - perhaps a
timing wheel or other construct would be more appropriate to handle the
actual timer value.
On Wed, Jun 29, 2022 at 9:08 AM Matthew Miller ***@***.***>
wrote:
> Let me take a swing at doing it within the SDK. The main challenge is
> scheduled executors only working with core threads, and core threads not
> being reused (even if idle), but we can probably work around that with a
> couple of executors or something more clever.
>
> It would be nice if everyone got this improvement, not just people 'in
> the know'.
>
> —
> Reply to this email directly, view it on GitHub
> <#3260 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AR5RI7ZLMXSNKOEUBUOF4Z3VRRYJPANCNFSM5ZNCHWXA>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
Oh yeah, jitter is a good idea, didn't think about that. |
|
My swing: #3275 If it looks good to you, did you want to try it out to make sure you see the resource consumption reduction you expect? |
|
@millems great work; added a few comments. Travelling atm (for the remainder of the week), won't be able to directly test, though the resource consumption improvements are evident in the removal of the per-cred-provider executor. Thanks for tackling this. |
|
I don't see your feedback on the PR. Did you miss hitting publish, or am I blind? |
|
S/b a couple of comments on there, let me check…
…On Thu, Jun 30, 2022 at 11:21 AM Matthew Miller ***@***.***> wrote:
I don't see your feedback on the PR. Did you miss hitting publish, or am I
blind?
—
Reply to this email directly, view it on GitHub
<#3260 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR5RI76QAVZCMVOVYY6YL33VRXQULANCNFSM5ZNCHWXA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Can’t see them atm - afk for a few hours, will drop them in again.
…On Thu, Jun 30, 2022 at 11:21 AM Matthew Miller ***@***.***> wrote:
I don't see your feedback on the PR. Did you miss hitting publish, or am I
blind?
—
Reply to this email directly, view it on GitHub
<#3260 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR5RI76QAVZCMVOVYY6YL33VRXQULANCNFSM5ZNCHWXA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow StsCredentialsProvider builder to take a ScheduledExecutorService for more efficient background processing.
Motivation and Context
See #3259.
Reduces the number of threads required for async refresh of credentials. In multi-tenant setups there can be large numbers of StsCredentialsProvider instances - maintaining a background thread for each one is unnecessary and expensive; this change allows for a shared ScheduledExecutorService to be used to reduce resource consumption.
Modifications
Provide additional constructors on software.amazon.awssdk.utils.cache.NonBlocking to take a ScheduledExecutorService (instead of creating one).
Modify software.amazon.awssdk.utils.cache.NonBlocking.close() to only cleanup executor service if owned (created) by this instance.
Modify software.amazon.awssdk.services.sts.auth.StsCredentialsProvider to allow a ScheduledExecutorService to be provided via the builder. When provided, the appropriate NonBlocking constructor is used.
Testing
Added additional test cases mirroring existing NonBlocking test cases to confirm no change in behaviour.
Screenshots (if appropriate)
n/a
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License