Feature/ri 3527 lua scripts #1386
Conversation
| import { DatabaseRecommendationsModule } from 'src/modules/db-recommendations/database-recommendations.module'; | ||
|
|
||
| @Module({ | ||
| imports: [DatabaseRecommendationsModule], |
There was a problem hiding this comment.
it is better to use the same names pattern for module and everything inside. e.g.
- DatabaseRecommendationsModule, DatabaseRecommendationsService, etc OR
- RecommendationsModule and RecommendationsService, etc.
or you can mix these names in case when you will create general RecommendationModule which might have database recommendations or some other kind of recommendations. So it might be nice to have RecommendationsModule and DatabaseRecommendationsService inside
| throw new NotFoundException(ERROR_MESSAGES.DATABASE_ANALYSIS_NOT_FOUND); | ||
| } | ||
|
|
||
| console.log(classToClass(DatabaseAnalysis, await this.decryptEntity(entity, true))) |
| databaseId: clientOptions.instanceId, | ||
| ...dto, | ||
| progress, | ||
| recommendations: await this.recommendationsService.getRecommendations(clientOptions), |
There was a problem hiding this comment.
I don't like such implementation for calculating recommendations. Current implementation is suitable for any recommendations based on info. What you are planning to do with this for other types of recommendations which should use keys analysis? (fetch again N keys?)
this implementation does fit current requirements for lua. so you, probably could go with this for now but this should be reworked a lot with next tasks related to recommendations
| return get(info, 'memory.number_of_cached_scripts', {}); | ||
| })); | ||
|
|
||
| return max(await nodesNumbersOfCachedScripts) > minNumberOfCachedScripts; |
There was a problem hiding this comment.
I don't like to do extra calls to Redis server since we already did it few seconds ago for other purpose. This is not critical but may be if you rework entire solution (see comment above) how we are calculating recommendations these unnecessary calls might be removed at all.
| // redis client (or info) | ||
| // constructor( | ||
| // private readonly databaseConnectionService: DatabaseConnectionService, | ||
| // ) {} |
There was a problem hiding this comment.
please, remove commented lines here
| public async getRecommendations( | ||
| redisClient: Redis, | ||
| // { client?, keys?, info?, etc } | ||
| // clientOptions: IFindRedisClientInstanceByOptions, |
| new Command('info', ['memory'], { replyEncoding: 'utf8' }), | ||
| ) as string, | ||
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); |
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); | ||
|
|
||
| return parseInt(await nodesNumbersOfCachedScripts, 10) < minNumberOfCachedScripts; |
| // ) as string, | ||
| // ); | ||
| // return get(info, 'memory.number_of_cached_scripts', {}); | ||
| // })); |
| } | ||
|
|
||
| @Injectable() | ||
| export class RecommendationService { |
There was a problem hiding this comment.
do you already have the same logic in the provider?
| new Command('info', ['memory'], { replyEncoding: 'utf8' }), | ||
| ) as string, | ||
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); |
There was a problem hiding this comment.
the same as above
There was a problem hiding this comment.
@AmirAllayarovSofteq Again, why default value is empty object here? Please, explain or fix it, do not ignore
get(info, 'memory.number_of_cached_scripts', {})
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); | ||
|
|
||
| return checkIsGreaterThan(minNumberOfCachedScripts, parseInt(await nodesNumbersOfCachedScripts, 10)); |
There was a problem hiding this comment.
the same as above
There was a problem hiding this comment.
@AmirAllayarovSofteq Why await here?
There was a problem hiding this comment.
Looks very good! I like how it implemented now.
However I have few concerns here:
- Weak tests. I did't see (correct me if I'm wrong) tests for recommendation creation (
.eq(0)only). When we must check that it is created and stored encrypted. - Please, address all comments before next review request. Reviewers might be wrong and it is normal to add your own comments if you don't agree with them and you are not going to change anything. Please do not ignore them. Review process itself might take a time and concentration. So I assume I have to go through your changes again, for this PR...
| new Command('info', ['memory'], { replyEncoding: 'utf8' }), | ||
| ) as string, | ||
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); |
There was a problem hiding this comment.
@AmirAllayarovSofteq Again, why default value is empty object here? Please, explain or fix it, do not ignore
get(info, 'memory.number_of_cached_scripts', {})
| ); | ||
| const nodesNumbersOfCachedScripts = get(info, 'memory.number_of_cached_scripts', {}); | ||
|
|
||
| return checkIsGreaterThan(minNumberOfCachedScripts, parseInt(await nodesNumbersOfCachedScripts, 10)); |
There was a problem hiding this comment.
@AmirAllayarovSofteq Why await here?
| const { client, keys, info } = dto; | ||
| const recommendations = []; | ||
| if (await this.recommendationProvider.determineLuaScriptRecommendation(client)) { | ||
| recommendations.push({ name: 'luaScript' }); |
There was a problem hiding this comment.
This is not something to implement right now but in the future if we want to have more fields inside recommendation (e.g. current value of cached scripts) to show it inside recommendation box on UI, it might be better whendetermineLuaScriptRecommendation method returns Recommendation model or null rather then boolean. May be refactor this with the next recommendations implementation
There was a problem hiding this comment.
I will think about it
| export const checkIsGreaterThan = ( | ||
| conditionNumber: number, | ||
| currentCount: number, | ||
| ): boolean => currentCount > conditionNumber; |
There was a problem hiding this comment.
Why we need this method? Do you plan to have custom logic inside? Do you plan to have a method for each possible condition???
There was a problem hiding this comment.
@arthosofteq We discussed this, there was a plan to separate the method into a function that takes cached_scripts_condition and current_cached_scripts and a function to calculate current_cached_scripts
…om/RedisInsight/RedisInsight into feature/RI-3527_lua_scripts
…void-dynamic-lua-script
…om/RedisInsight/RedisInsight into feature/RI-3527_lua_scripts
…ndations_message #RI-4023 - update no recommendations message
…ndation_voting #RI-3977 - add recommendation voting
…ndation_voting #RI-3977 - add recommendation voting
Delete unused nth from verifyVoteDisabled
Add assert message
Delete .only
…ecommendations Feature/ri 3971 3572 add recommendations
Add comments to methods
…endation_voting E2e/feature/ri 3977 recommendation voting
…tion_voting Feature/ri 3977 recommendation voting
…nto feature/RI-3527_lua_scripts
RI-3527 - add luaScript recommendation
RI-3565 - add use smaller keys recommendation
RI-3566 - add big hashes recommendation