You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2017/05/12 19:01:47 UTC
Review Request 59239: Allow a locator host to be taken off line and
replaced with a different machine having the same host name
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/
-----------------------------------------------------------
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 b3de975
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java 385569c
Diff: https://reviews.apache.org/r/59239/diff/1/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review174997
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
Lines 175 (patched)
<https://reviews.apache.org/r/59239/#comment248307>
Maybe it should say "AutoConnectionSourceImpl.addBadLocators"?
- Bruce Schuchardt
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
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On May 15, 2017, 7:43 p.m., Galen O'Sullivan wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Lines 217 (patched)
> > <https://reviews.apache.org/r/59239/diff/2/?file=1718520#file1718520line219>
> >
> > 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);`
Good Catch. I have removed that log, otherwise it will fill the log file
> On May 15, 2017, 7:43 p.m., Galen O'Sullivan wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Lines 237 (patched)
> > <https://reviews.apache.org/r/59239/diff/2/?file=1718520#file1718520line239>
> >
> > 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.
Updated
> On May 15, 2017, 7:43 p.m., Galen O'Sullivan wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Lines 248 (patched)
> > <https://reviews.apache.org/r/59239/diff/2/?file=1718520#file1718520line250>
> >
> > This could be written as
> >
> > `for (InetSocketAddress tloc : locatorList.getLocators())`
> >
> > and `locIterator` declaration removed.
updated
> On May 15, 2017, 7:43 p.m., Galen O'Sullivan wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Lines 250 (patched)
> > <https://reviews.apache.org/r/59239/diff/2/?file=1718520#file1718520line252>
> >
> > 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.
Not sure why I had added that but I put that check up.
> On May 15, 2017, 7:43 p.m., Galen O'Sullivan wrote:
> > geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
> > Lines 155 (patched)
> > <https://reviews.apache.org/r/59239/diff/2/?file=1718525#file1718525line155>
> >
> > Is floc2 necessary?
It just to test the logic.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review175000
-----------------------------------------------------------
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
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On May 15, 2017, 6:45 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Line 269 (original), 308 (patched)
> > <https://reviews.apache.org/r/59239/diff/1/?file=1717126#file1717126line310>
> >
> > It might be just me, but I don't see how the `initialLocators` list is suddenly bad. We should just treat it as an `initialList`. Why would they suddenly be `bad`?
We got new locator list from live locator and it doesn't contain the initial locator.
> On May 15, 2017, 6:45 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
> > Lines 351 (patched)
> > <https://reviews.apache.org/r/59239/diff/1/?file=1717126#file1717126line353>
> >
> > What makes a locator "bad"? I would argue that we make this method more of a general "mergeLocatorLists". The fact that we add list containing "bad" locators to another list containing locators, is circumstantial.
We could change name of that method but I think it is giving you intent of that method explicitly. And thats how its define in code.
> On May 15, 2017, 6:45 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
> > Lines 168 (patched)
> > <https://reviews.apache.org/r/59239/diff/1/?file=1717131#file1717131line168>
> >
> > Maybe this becomes a test that we can successfully merge 2 locator lists.
We just validaing method logic.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review174980
-----------------------------------------------------------
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
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review174980
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Line 269 (original), 308 (patched)
<https://reviews.apache.org/r/59239/#comment248287>
It might be just me, but I don't see how the `initialLocators` list is suddenly bad. We should just treat it as an `initialList`. Why would they suddenly be `bad`?
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 351 (patched)
<https://reviews.apache.org/r/59239/#comment248285>
What makes a locator "bad"? I would argue that we make this method more of a general "mergeLocatorLists". The fact that we add list containing "bad" locators to another list containing locators, is circumstantial.
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 353-354 (patched)
<https://reviews.apache.org/r/59239/#comment248283>
maybe this can be replaced with
`for (InetSocketAddress badLocator: badLocators)
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
Lines 168 (patched)
<https://reviews.apache.org/r/59239/#comment248308>
Maybe this becomes a test that we can successfully merge 2 locator lists.
- Udo Kohlmeyer
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
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On May 15, 2017, 10:10 p.m., Udo Kohlmeyer wrote:
> > geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
> > Lines 148 (patched)
> > <https://reviews.apache.org/r/59239/diff/3/?file=1719293#file1719293line148>
> >
> > It isn't immediately evident why `floc1` is not equal to the `floc1` that is updated in `src.updateLocatorInLocatorList`.
> > Also maybe a `==` instance comparison is better than identityHashCode
I will change that. Thanks Bruce and Udo.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review175022
-----------------------------------------------------------
On May 15, 2017, 8:46 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, 8:46 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review175022
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Line 269 (original), 307 (patched)
<https://reviews.apache.org/r/59239/#comment248339>
Why are these locators "bad"... It is just the initial configured list of locators..
geode-core/src/main/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImpl.java
Lines 350 (patched)
<https://reviews.apache.org/r/59239/#comment248338>
Why do we name this "badLocators"? Really it's is a merge of 2 lists of InetSocketAddress...
geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java
Lines 148 (patched)
<https://reviews.apache.org/r/59239/#comment248343>
It isn't immediately evident why `floc1` is not equal to the `floc1` that is updated in `src.updateLocatorInLocatorList`.
Also maybe a `==` instance comparison is better than identityHashCode
- Udo Kohlmeyer
On May 15, 2017, 8:46 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, 8:46 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/
-----------------------------------------------------------
(Updated May 15, 2017, 8:46 p.m.)
Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo Kohlmeyer.
Changes
-------
Updated the review comment
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/59239/diff/2-3/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
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.
Changes
-------
Updated comments on test
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/59239/diff/1-2/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On May 12, 2017, 9:53 p.m., Bruce Schuchardt wrote:
> > It would be nice to have a javadoc on the two tests. Other than that the changes look good to me Hitesh.
I will update that. Thanks.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review174861
-----------------------------------------------------------
On May 12, 2017, 7:01 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59239/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 7:01 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 b3de975
> geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java 385569c
>
>
> Diff: https://reviews.apache.org/r/59239/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 59239: Allow a locator host to be taken off line
and replaced with a different machine having the same host name
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59239/#review174861
-----------------------------------------------------------
Ship it!
It would be nice to have a javadoc on the two tests. Other than that the changes look good to me Hitesh.
- Bruce Schuchardt
On May 12, 2017, 7:01 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59239/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 7:01 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 b3de975
> geode-core/src/test/java/org/apache/geode/cache/client/internal/AutoConnectionSourceImplJUnitTest.java 385569c
>
>
> Diff: https://reviews.apache.org/r/59239/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>