feat(wip): added cached entity #2489
Merged
Conversation
This file contains 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
djabarovgeorge
commented
Jan 12, 2023
apps/api/src/app/shared/services/cache/invalidate-cache.service.ts
Outdated
Show resolved
Hide resolved
|
|
@scopsy Created caching only by subscriber id. |
scopsy
reviewed
Jan 15, 2023
Comment on lines
60
to
69
| @CachedEntity({ | ||
| builder: KeyGenerator.subscriber, | ||
| }) | ||
| private async fetchSubscriber({ | ||
| subscriberId, | ||
| _environmentId, | ||
| }: { | ||
| subscriberId: string; | ||
| _environmentId: string; | ||
| }): Promise<SubscriberEntity | null> { |
There was a problem hiding this comment.
Can we enforce the param, passed to the function to make sure that it's enforce by TS? That hsubscriberId and environment id is indeed passed?
There was a problem hiding this comment.
Tried it about a week ago and a bit struggled to do so.
Comment on lines
62
to
70
| const buildQueryKey = ({ | ||
| type, | ||
| keyPrefix, | ||
| environmentIdPrefix = 'e', | ||
| environmentId, | ||
| identifierPrefix = 'i', | ||
| identifier, | ||
| query, | ||
| }: { |
There was a problem hiding this comment.
Would be great to have those unit tested
# Conflicts: # .cspell.json # apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message-chat.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message-email.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message-push.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message.command.ts # apps/api/src/app/events/usecases/send-message/send-message.usecase.ts # apps/api/src/app/shared/services/cache/cache.service.ts # apps/api/src/app/widgets/usecases/get-notifications-feed/get-notifications-feed.usecase.ts # apps/api/src/app/widgets/usecases/mark-all-message-as-seen/mark-all-message-as-seen.usecase.ts # apps/api/src/app/widgets/usecases/mark-message-as/mark-message-as.usecase.ts
👇 Click on the image for a new way to code review
Legend |
|
@djabarovgeorge Note this on the PR progress https://github.com/novuhq/novu/pull/2754/files#r1105489639 |
# Conflicts: # apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts # apps/api/src/app/events/usecases/send-message/send-message.usecase.ts # apps/api/src/app/events/usecases/trigger-event/message-matcher.service.spec.ts # apps/api/src/app/subscribers/usecases/get-subscriber-template-preference/get-subscriber-template-preference.command.ts # apps/api/src/app/subscribers/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts # apps/api/src/app/subscribers/usecases/update-subscriber-preference/update-subscriber-preference.usecase.ts
djabarovgeorge
commented
Feb 18, 2023
Comment on lines
21
to
29
| export class KeyGenerator { | ||
| public static query = () => { | ||
| return { feed, messageCount }; | ||
| }; | ||
|
|
||
| public static entity = () => { | ||
| return { subscriber, integration, notificationTemplate, user, environmentByApiKey }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This one is a POC I am thinking about what could be the easiest way to interact with the methods, with IDE suggestion of the class methods
feat: refactor cache functions
…rigger feat: cache notification template by trigger identifier
scopsy
reviewed
Mar 30, 2023
| } | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`An error has occurred when extracting "key: ${methodName}`, 'CacheInterceptor', err); |
There was a problem hiding this comment.
Suggested change
| console.error(`An error has occurred when extracting "key: ${methodName}`, 'CacheInterceptor', err); | |
| Logger.error(`An error has occurred when extracting "key: ${methodName}`, 'CacheInterceptor', err); |
scopsy
approved these changes
Mar 30, 2023
| const variant = this.TTL_VARIANT_PERCENTAGE * num * Math.random(); | ||
|
|
||
| return Math.floor(num - (this.TTL_VARIANT_PERCENTAGE * num) / 2 + variant); | ||
| } | ||
|
|
||
| private splitKey(key: string) { |
There was a problem hiding this comment.
Would be great to add test here
# Conflicts: # apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts # apps/api/src/app/topics/use-cases/index.ts
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.
What change does this PR introduce?
It is the POC of dividing the Cached Interceptor into CachedRequest and CachedEntity, this one is the CachedEntity part, I a bit carried away and added a couple of extra queries which were not so necessary at the moment please look at the trigger flow part.
In this solution, we cache under subscriber._id && subscriber.subscriberId separately, and when we need to invalidate we invalidate by key and not by pattern.
Why was this change needed?
In order to reduce the number of scans on the Redis server.
Other information (Screenshots)