HostAndPort.convertHost() to resolve localhost in a lazy manner #1468
Conversation
|
There is some issue with redis dependencies in Travis. Tests passed in my local with |
|
LGTM @HeartSaVioR, @allanwax ? |
|
@jkrahul can you rebase with master so tests pass please? |
| return host; | ||
| } | ||
| } | ||
|
|
||
| public static void setLocalhost(String localhost) { |
There was a problem hiding this comment.
Are there any actual usages for setLocalhost()? setLocalhost() and getLocalhost() can make race condition.
Btw, getLocalhost() itself can make race condition but it might be OK given that getLocalHostQuietly() always returns same value.
There was a problem hiding this comment.
@marcosnils @jkrahul The only use I can think of is a potential one. If the programmer wants to override the natural local host value, say with the ENDPOINT value, then they can make this call change it. This would be done before actually creating a Jedis instance.
There was a problem hiding this comment.
Wrapping the operations inside setLocalhost and getLocalhost will make it safe and is less heavy weight than synchronized.
There was a problem hiding this comment.
@marcosnills @jkrahul I was imagining the wrapping being a ReentrantLock. I'm good .
|
@jkrahul I've done reviewing and left comment. |
|
@jkrahul Please rebase and address review comment. Thanks in advance! |
jkrahul
force-pushed
the
dev-resolve-localhost
branch
from
2541c82
to
f2f617d
Compare
|
@marcosnils @HeartSaVioR @allanwax review comments have been addressed. Please check |
|
I believe we're good now. @allanwax @HeartSaVioR ? |
|
@marcosnils Yes LGTM. |
|
Merged to master and 2.10 respectively. @jkrahul Thanks for contribution! |
fixes #1424