-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
optimization : merge two redis calls in api key authenticate #3982
optimization : merge two redis calls in api key authenticate #3982
Conversation
@@ -32,6 +32,7 @@ export enum CacheKeyPrefixEnum { | |||
INTEGRATION = 'integration', | |||
ENVIRONMENT_BY_API_KEY = 'environment_by_api_key', | |||
GROUPED_BLUEPRINTS = 'grouped-blueprints', | |||
USER_DATA = 'user_data_env_entity_user_entity', |
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.
USER_DATA = 'user_data_env_entity_user_entity', | |
AUTH_SERVICE = 'auth_service', |
Mainly because it is not our User data entity and we try to return multiple stuff, all related to a certain service.
A better name might be found than my suggestion, I reckon.
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 wanted to be more informative but awesome suggestion IMO thanks!
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.
🌟
Perf improvements always welcome.
@@ -335,4 +337,31 @@ export class AuthService { | |||
private async getSubscriber({ subscriberId, _environmentId }: { subscriberId: string; _environmentId: string }) { | |||
return await this.subscriberRepository.findBySubscriberId(_environmentId, subscriberId); | |||
} | |||
|
|||
@CachedEntity({ |
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.
@djabarovgeorge what is our current default TTL?
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.
@scopsy I kept it as it is now (environment and user), it is 2 hours.
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.
We should clear it when the user email
has changed, like in this use-case UpdateProfileEmail
. Otherwise, the signed token might have an old email.
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.
🥇
What change does this PR introduce?
merge two redis calls in api key to authenticate
Why was this change needed?
in order to reduce the redis call in trigger high load
Other information (Screenshots)