feat: cache notification template by trigger identifier #3074
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
p-fernandez
approved these changes
Mar 27, 2023
Comment on lines
194
to
202
| /* | ||
| * const notificationTemplate = ({ _id, _environmentId }: { _id: string; _environmentId: string }): string => | ||
| * buildCommonKey({ | ||
| * type: CacheKeyTypeEnum.ENTITY, | ||
| * keyEntity: CacheKeyPrefixEnum.NOTIFICATION_TEMPLATE, | ||
| * environmentId: _environmentId, | ||
| * identifier: _id, | ||
| * }); | ||
| */ |
| identifiers, | ||
| }: { | ||
| _environmentId: string; | ||
| identifiers: ({ id: string } & { triggerIdentifier?: string }) | ({ id?: string } & { triggerIdentifier: string }); |
There was a problem hiding this comment.
I have been trying to find a way to be able to express mutually exclusive optional properties but I haven't been able to found it. Not sure if something along these lines would be possible but changed to make the other property optional.
microsoft/TypeScript#14094 (comment)
feat: refactor cache functions
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?
Created this approach in order to make sure we store in cache queries by fetching by triggerIdentifier.
So how it will work:
In order to make sure that we can store the notification-template both by id and triggerIdentifier, I reused the CachedQuery logic, meaning that all the queries will be stored as queries (the fetch will be cache get O(1))
Key structure:
template: {type}:{keyEntity}:{environmentIdPrefix}={environmentId}:{QUERY_PREFIX}={JSON.stringify(query)}
example: query:notification-template:e=12345678:QUERY_PREFIX={json: example}
But now the invalidation will be the same as on CachedQuery, meaning we will store all the notification-templates that are in the same environment( + the duplication one by id and one by triggerIdentifier), and on invalidate we will delete all of the available notification-templates stored in the cache in the same environment.( cache get hset O(1), create pipeline and loop all the key in order to delete them, pipeline execute) that way we will make sure that all the stale cache is deleted.
I chose this solution because it will produce the most accurate data for our users, the other solution is to remove this suggestion and adjust the TTL in order to handle the stale cache but then we won't ensure that the data is updated in the cache storage.
TYI: I created this branch PR so the solution will be isolated, but please note that the new logic is still the WIP, I will create a new branch from this one with the refactoring.
Why was this change needed?
In order to store the notification-templates findByTriggerIdentifier queries on the trigger flow.
Other information (Screenshots)