[Improve][Redis] Optimized Redis connection params #8841
Conversation
| if (config.getOptional(RedisBaseOptions.HOST).isPresent()) { | ||
| // set host | ||
| this.host = config.get(RedisBaseOptions.HOST); | ||
| } | ||
| if (config.getOptional(RedisBaseOptions.PORT).isPresent()) { | ||
| // set port | ||
| this.port = config.get(RedisBaseOptions.PORT); | ||
| } |
There was a problem hiding this comment.
The behavior before is same as this patch. Because when host or port not configured, config.get(RedisBaseOptions.HOST) will return null.
There was a problem hiding this comment.
The behavior before is same as this patch. Because when host or port not configured,
config.get(RedisBaseOptions.HOST)will return null.
Thank you for your review. I have tested locally that when the host and port are not configured, if the condition is judged as false, the situation you mentioned will not occur. Is that my understanding correct? However, I have discovered another issue that I will update and fix later
There was a problem hiding this comment.
I mean this part you changed the bahavior is same as before you change.
When host existed, get host value and set to this.host, when host not existed, the host value would set to null. So I think the change of this part is unnecessary.
There was a problem hiding this comment.
Okay, I understand what you mean. Thank you for your suggestion
|
LGTM |
|
During the testing process, I found an exception in the jedissWrapper. select (dbNum) method. I checked the official website and found that redisCluster does not support selecting db. If the select method is used, an error will be reported. Therefore, I believe this is a new issue. I will briefly raise an issue and try to fix it
|
5 participants
Purpose of this pull request
close #8836
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note.