Add cache-flush step in Twenty upgrade command #7521 #7553
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. TODOs/FIXMEs:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances the cache flushing functionality in the Twenty upgrade command, allowing for selective flushing of Redis cache keys based on a specified pattern.
- Added
flushByPatternmethod inCacheStorageServiceto selectively flush Redis keys matching a pattern - Updated
FlushCacheCommandto accept an optional--patternflag for targeted cache flushing - Implemented pattern-based flushing in
FlushCacheCommand, defaulting to full cache flush if no pattern is provided - Ensured compatibility with Redis cache, throwing an error for non-Redis caches in
flushByPattern - Potential performance concern: Use of Redis
keyscommand influshByPatternmay impact large datasets
2 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -20,10 +20,27 @@ export class FlushCacheCommand extends CommandRunner { | |||
| ) { | |||
| super(); | |||
| } | |||
|
|
|||
|
|
|||
| async run(passedParams: string[], options?: Record<string, any>): Promise<void> { | |||
There was a problem hiding this comment.
style: Consider adding type annotations for 'passedParams' and 'options' parameters
|
|
||
|
|
||
| async run(passedParams: string[], options?: Record<string, any>): Promise<void> { | ||
| const pattern = options?.pattern || '*'; // Default to flushing all keys if no pattern is passed |
There was a problem hiding this comment.
style: Consider using nullish coalescing (??) instead of logical OR (||) for better type safety
| parsePattern(val: string): string { | ||
| return val; | ||
| } |
There was a problem hiding this comment.
style: parsePattern method can be simplified to a one-liner arrow function
| async flushByPattern(pattern: string): Promise<void> { | ||
| if (!this.isRedisCache()) { | ||
| throw new Error('flushByPattern is only supported with Redis cache'); | ||
| } | ||
|
|
||
| const redisClient = (this.cache as RedisCache).store.client; | ||
|
|
||
|
|
||
| const keys = await redisClient.keys(`${this.namespace}:${pattern}`); | ||
| if (keys.length > 0) { | ||
| await redisClient.del(keys); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
style: Consider using SCAN instead of KEYS for better performance in production environments with large datasets
| if (keys.length > 0) { | ||
| await redisClient.del(keys); | ||
| } |
There was a problem hiding this comment.
style: Consider using UNLINK instead of DEL for non-blocking key deletion in Redis versions that support it
|
@Weiko Thank you for your feedback |
|
/award 300 |
|
Awarding yadavshubham01: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/yadavshubham01 |
…q#7553) Flush Specific Redis Keys After Upgrade Command :---- Changes Made :---- Updated the FlushCacheCommand to allow for selective flushing of keys that match the pattern engine:*, rather than flushing the entire cache. Added a new optional parameter to specify the cache keys pattern. Ensured that the cache is flushed at the end of the upgrade command. Code Changes :---- Modified FlushCacheCommand to include a method for flushing keys by pattern. Added a new function in CacheStorageService to handle the pattern-based flushing. --------- Co-authored-by: Weiko <[email protected]>
Fixes: #7521
Flush Specific Redis Keys After Upgrade Command :----
Changes Made :----
Updated the FlushCacheCommand to allow for selective flushing of keys that match the pattern engine:*, rather than flushing the entire cache.
Added a new optional parameter to specify the cache keys pattern.
Ensured that the cache is flushed at the end of the upgrade command.
Code Changes :----
Modified FlushCacheCommand to include a method for flushing keys by pattern.
Added a new function in CacheStorageService to handle the pattern-based flushing.