Feature/ri 3971 3572 add recommendations #1590
Conversation
| ): Promise<Recommendation> { | ||
| try { | ||
| const dangerousCommands = await redisClient.sendCommand( | ||
| new Command('command', ['LIST', 'FILTERBY', 'aclcat', 'dangerous'], { replyEncoding: 'utf8' }), |
There was a problem hiding this comment.
This will not work for Redis < 7
| }); | ||
|
|
||
| const activeDangerousCommands = await Promise.all( | ||
| filteredDangerousCommands.map(async (command) => await this.checkCommandInfo(redisClient, command)), |
There was a problem hiding this comment.
What is the goal to additionally send command info <cmd> here?
Is it possible to use command without any args and filter all commands from that list?
| return false; | ||
| } | ||
|
|
||
| private async checkCommandInfo(redisClient: Redis | Cluster, command: string): Promise<string> { |
There was a problem hiding this comment.
I believe this method is not needed
|
|
||
| return isJSONOrHash ? { name: RECOMMENDATION_NAMES.SEARCH_INDEXES } : null; | ||
| } | ||
| const sortedSets = keys |
There was a problem hiding this comment.
Check types for standalone should be in separate method to decrease complexity.
There was a problem hiding this comment.
In general I'm fine with how you are processing search recommendations for standalone. nice!
| client: any, | ||
| ): Promise<Recommendation> { | ||
| try { | ||
| if (client.isCluster) { |
There was a problem hiding this comment.
Please move this to separate method. Let's have 2 separate method determineForCluster*... and determineForStandalone*...
| let processedKeysNumber = 0; | ||
| let isJSONOrHash = false; | ||
| let sortedSetNumber = 0; | ||
| while ( |
There was a problem hiding this comment.
while? why?
I think we should use here Promise.all to not wait before processing the next key.
In general I think that this implementation should be the same as for Standalone databases but with Promise.all -> sendCommand instead of piepeline
| new Command('type', [sortedSetMember[0]], { replyEncoding: 'utf8' }), | ||
| ) as string; | ||
| } catch (err) { | ||
| if (err && checkRedirectionError(err)) { |
There was a problem hiding this comment.
Are you sure that we need this?
As I remember we can simplify this "redirection" functionality a lot.
Let's use not node client but parent client (general cluster client) to execute commands. In this case all commands will be routed to needed node automatically, no? did you check this behaviour?
| processedKeysNumber += 1; | ||
| sortedSetNumber += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
In general I would like to see 2 separate methods for cluster and standalone but with almost the same logic except pipeline vs Promise.all sendCommand (on cluster client). Is it possible?
| recommendationToExclude.push(...foundedRecommendationNames); | ||
| jobsArray.push(foundedRecommendations); | ||
| return flatten(jobsArray); | ||
| }, Promise.resolve([])); |
There was a problem hiding this comment.
In general I understand this idea. It is not readable a little bit but should work. I would prefer to have Map with recommendations when each recommendation has own type and based on it we will check this recommendation or not. But let's revise this approach and refactor current solution when we will add batch or scan more functionality
…ndations_message #RI-4023 - update no recommendations message
4 participants
No description provided.