fix(amazonq): listAvailableProfile calls are throttled when users have 2+ IDE instances #7134
Conversation
|
|
Will-ShaoHua
changed the title
Will-ShaoHua
changed the title
|
will add a changelog |
| | 'aws.toolkit.lsp.manifest' | ||
| | 'aws.amazonq.customization.overrideV2' | ||
| | 'aws.amazonq.regionProfiles' | ||
| | 'aws.amazonq.regionProfiles.cachedResult' |
There was a problem hiding this comment.
why not set a flag on the items in aws.amazonq.regionProfiles ? why does this need an entirely separate structure?
There was a problem hiding this comment.
'aws.amazonq.regionProfiles'
this is to memorize previous selected option, so we will need a separate structure to cache previous api call response
There was a problem hiding this comment.
'aws.amazonq.regionProfiles'
this is to memorize previous selected option
but that could be a flag on the item(s) in the structure. why is a separate structure needed?
There was a problem hiding this comment.
that makes sense, i will reuse the existing one then
There was a problem hiding this comment.
@justinmk3 i encounter an issue or say concern when i am doing this exercise 3e50145#diff-f5716036a70a8b212e9dae829ec09e33731f181c173a3f2b9ad9d361d51af14bR54-R56, if I want to reuse the same global key, i might need to restructure the schema, in such case, users who already selected profile update their plugin, they might need to re-select the profile again. Given the sev2s we have right now, I am concerned if we want to do this now or is there another way to deal with such scenario?
There was a problem hiding this comment.
Definitely want to avoid a restructure. If a restructure is required, then I guess we need a new globalState field.
| }) | ||
| } | ||
|
|
||
| private async tryAcquireLock(): Promise<CachedApiResultWithLock | undefined> { |
There was a problem hiding this comment.
This is rather delicate logic. And it is straightforward to generalize it. Why is it buried in a random module?
Note that
attempted to achieve something similiar, except using the filesystem. Though it has some known issues.There was a problem hiding this comment.
btw Justin, i'm going to use #7151 to replace this pr for this purpose (if everything works just fine there), because the diff are pretty large so would be easier to just have another PR.
justinmk3
left a comment
justinmk3
left a comment
There was a problem hiding this comment.
Although this is urgent work, it's also urgent that it works correctly. So separating it into a shared module will help with that. Especially if bugs are discovered and must be debugged.
Will-ShaoHua
changed the title
|
replace with #7151 |
…hey have 2+ vscode instances open (#7151) ## Problem revision of #7134, this pr aims to address the comment from the previous PR #7134 (comment) to extract the logic to a shared module so the diff against 7134 is expected to be large. after deployment 4/21, service has a strict 1 tps throttling policy and there was no caching for the API call previously. It will impact users as long as they have multiple ide instances opened as all of them will make list profile call while users attempt to sign in. ## Solution 1. cache the api result for 60 seconds for reuse 2. use lock to prevent multiple instances trying to call at the same time. Only 1 will make the service call and the rest will wait until it's done and read from the cache. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
2 participants
Problem
after deployment 4/21, service has a strict 1 tps throttling policy and there was no caching for the API call previously.
It will impact users as long as they have multiple ide instances opened as all of them will make list profile call while users attempt to sign in.
Solution
feature/xbranches will not be squash-merged at release time.