----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59546/#review176020 -----------------------------------------------------------
Good work! I have a few minor comments and a couple of things I'm concerned about because I don't understand the rest of the system so well. Maybe you can help clarify for me. It looks like the host will be set when we call `getHost()`. Do we want to expire hosts or give up looking for them? Are callers expecting an existing locator to be valid and connected? Will this cause delays as we iterate through locators that have been dropped or were never up? I remember there being a ticket recently to save hostnames for Cache members and pass the hostnames from the Locator. I'm wondering if it would make sense to have a shared `lazilyEvaluatedHost` class? I don't think that should hold this up, though. geode-core/src/main/java/org/apache/geode/admin/internal/DistributionLocatorImpl.java Line 196 (original), 196 (patched) <https://reviews.apache.org/r/59546/#comment249359> I'd rather this code be deleted than commented out. Can you explain why we're using the hostname rather than the host? geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java Line 1555 (original), 1555 (patched) <https://reviews.apache.org/r/59546/#comment249424> Will this mess with anything that depends on the canonical locator serialization? geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java Line 131 (original), 132 (patched) <https://reviews.apache.org/r/59546/#comment249425> It looks like taking out this exception will cause us to behave differently if we can't look up the hostname. There's a lot going on here. Are we trying to retain the hostname for reconnects or keep trying to connect here? geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java Lines 234 (patched) <https://reviews.apache.org/r/59546/#comment249426> `host` can be null, see line 131 of DistributionLocatorId.java. It looks like `hostname` can't be null if we call the `String` constructor, but it can if we call the IP and port constructor. I would like to unify the constructors if we can, because it looks like the string-only form works differently from the other form, but it's not clear what the constraints on `bindAddress` are in the other form. geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java Lines 278 (patched) <https://reviews.apache.org/r/59546/#comment249433> What makes the new code able to throw an exception here? We could still specify locators by IP before, right? geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java Lines 346 (patched) <https://reviews.apache.org/r/59546/#comment249432> Could this be split into two tests, one of which continues and the other of which triggers a failure later on? - Galen O'Sullivan On May 25, 2017, 5:12 p.m., Hitesh Khamesra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59546/ > ----------------------------------------------------------- > > (Updated May 25, 2017, 5:12 p.m.) > > > Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo > Kohlmeyer. > > > Repository: geode > > > Description > ------- > > We configure locator list to start the cache. This locator list is validated > while creating the cache. We verify whether locator host exist or not. Now we > have remove this verification as in cloud environment host may not available > for time being. > > Patch from Bruce. Modified couple of tests. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/admin/internal/DistributionLocatorImpl.java > c1bfc93 > > geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java > 01c6157 > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > 7caad3f > > geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java > 5ab1bed > > geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java > 1dc2fd1 > > geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java > dc73f04 > > geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java > 9f6c5fb > > geode-core/src/test/resources/org/apache/geode/codeAnalysis/excludedClasses.txt > 9cff80d > > geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorMembershipListenerImpl.java > d6d5d7c > > > Diff: https://reviews.apache.org/r/59546/diff/1/ > > > Testing > ------- > > > Thanks, > > Hitesh Khamesra > >
