geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Galen O'Sullivan <gosulli...@pivotal.io>
Subject Re: Review Request 59239: Allow a locator host to be taken off line and replaced with a different machine having the same host name
Date Mon, 15 May 2017 19:43:29 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review175000
-----------------------------------------------------------




geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 217 (patched)
<https://reviews.apache.org/r/59239/#comment248309>

    The exception should be logged in `reportDeadLocator`. Although perhaps the message in
there should be changed from "locator {0} is not running" to "could not connect to locator
{0}".
    
    If we keep the log statement, I think it's better to use the other form `logger.info("queryOneLocator
got Exception ", ioe);`



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 237 (patched)
<https://reviews.apache.org/r/59239/#comment248313>

    If connecting to the locator fails with an `IOException`, this may be because the locator's
IP has changed. Add the locator back to the list of locators using host address rather than
IP. This will cause another DNS lookup, hopefully finding the locator.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 243 (patched)
<https://reviews.apache.org/r/59239/#comment248310>

    This comment should be deleted.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 248 (patched)
<https://reviews.apache.org/r/59239/#comment248311>

    This could be written as 
    
    `for (InetSocketAddress tloc : locatorList.getLocators())`
    
    and `locIterator` declaration removed.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 250 (patched)
<https://reviews.apache.org/r/59239/#comment248321>

    In the case where the locator isn't in the list, do we mean to add it to `newLocatorList`
or discard it?
    
    If we mean to add it, we should move the check `locator.getHostName() != null` to above
the loop.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 257 (patched)
<https://reviews.apache.org/r/59239/#comment248312>

    typo: "loc from" instead of "loc form"
    
    ideally comma comes before space in log messages, or I would just say `"from: " + locator
+ " to: "`.



geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 354 (patched)
<https://reviews.apache.org/r/59239/#comment248314>

    I think this is a good candidate for the "enhanced for statement" sytax (`for (badLoc
: badLocators) {`)



geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
Line 141 (original), 142 (patched)
<https://reviews.apache.org/r/59239/#comment248316>

    I think you copied the wrong comment here -- we arent' expecting a reply if we take a
`replyExpected` parameter.
    
    Same with the comment on line 149/150.



geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
Lines 155 (patched)
<https://reviews.apache.org/r/59239/#comment248318>

    Is floc2 necessary?


- Galen O'Sullivan


On May 15, 2017, 6:33 p.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59239/
> -----------------------------------------------------------
> 
> (Updated May 15, 2017, 6:33 p.m.)
> 
> 
> Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo Kohlmeyer.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2804 Update InetSocketAddress, when there is IOException.
> 
>     Geode keeps InetSocketAddress for locators. So, if locators ip
>     changes in cloud/VM enviroment then Geode process unable to
>     connect to locator. Thus we have fixed this problem in two way.
> 
>     a. If Geode client sees IOException while connectin to locator then
>     we change cached InetAddress to use locator hostname. In this way,
>     client does DNS query again for locator host.
>     b. For other Geode process, now we connect to locator using hostname.
> 
>     Added couple of junit tests for it.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
1e807ee 
>   geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
4bf010b 
>   geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
6b54170 
>   geode-core/src/main/java/org/apache/geode/management/internal/JmxManagerLocatorRequest.java
0efba01 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
d61e72d 
>   geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
385569c 
> 
> 
> Diff: https://reviews.apache.org/r/59239/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message