Conversation
# Conflicts: # .gitignore # core-tests/e2e-tests/pom.xml
| .filter(command -> command.getType().shouldCalculateHeuristic()) | ||
| .forEach(redisCommand -> { | ||
| RedisDistanceWithMetrics distanceWithMetrics = computeDistance(redisCommand, redisClient); | ||
| RedisDistanceWithMetrics distanceWithMetrics = computeDistance(redisCommand, (RedisClient) redisClient); |
There was a problem hiding this comment.
hi. thx @aszyrej i think it is best if @jgaleotti gives first round of comments, and then, after updating, i ll give my pass. but one thing here that caught my eyes is: why turning RedisClient into Object reference if anyway you do a cast to (RedisClient) here?
There was a problem hiding this comment.
Hi @arcuri82 . The reason why I'm using Object is just to be aligned with the solutions for Mongo and OpenSearch where they used Object for the clients. It's not necessary to keep it this way and since I have a dedicated class for encapsulating the client logic, I could refactor it and keep that class instead of Object.
I'll proceed with this if you agree.
There was a problem hiding this comment.
the reason for using private Object mongoClient is that cannot have dependencies to actual Mongo code, and must do access via reflection. if for Redis you encapsulate all the code for reflection in your own RedisClient that is different from io.lettuce.core.RedisClient, then no point in storing it as an Object reference. Btw, for clarity, your own RedisClient could be renamed into ReflectionBasedRedisClient (or something like that)
There was a problem hiding this comment.
Hi @arcuri82, I've renamed that class into ReflectionBasedRedisClient as we discussed, and changed the type in RedisHandler.
|
hi @jgaleotti it would be best if you look at this PR first, and then, after you approve it, i review it as well |
jgaleotti
left a comment
jgaleotti
left a comment
There was a problem hiding this comment.
Please review the use of string literals and the maximum distance value
jgaleotti
left a comment
jgaleotti
left a comment
There was a problem hiding this comment.
changes asked in pom.xml
jgaleotti
left a comment
jgaleotti
left a comment
There was a problem hiding this comment.
No further changes from my side. Please request Andrea's revision.
|
@arcuri82 would you review this PR and let me know if we're ready to merge? Thanks. |
…n Mongo handler initialization
…troller - initRedisHandler
|
@arcuri82 Would you please verify that everything is in order now and we're ready to merge? |
|
@aszyrej thx. sorry for delay, but i was sick the whole of last week... :( |
3 participants
First e2e-test for Redis.
Includes GET and SET commands, applying Redis heuristics.