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/24 22:30:56 UTC
Review Request 59546: GEODE-2940 Remove verification of locator host
on start
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo Kohlmeyer.
Repository: geode
Description
-------
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
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/#review176013
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
Lines 234 (patched)
<https://reviews.apache.org/r/59546/#comment249355>
we seem to assume here that `this.host != null`... the same cannot be said for the `getHost()` method
- Udo Kohlmeyer
On May 24, 2017, 10:30 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59546/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 10:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> 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
>
>
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Galen O'Sullivan <go...@pivotal.io>.
> On May 25, 2017, 5:51 p.m., Galen O'Sullivan wrote:
> > geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
> > Lines 346 (patched)
> > <https://reviews.apache.org/r/59546/diff/1/?file=1732164#file1732164line346>
> >
> > Could this be split into two tests, one of which continues and the other of which triggers a failure later on?
That is, could one of the tests test what happens when we try to evaluate the host later? (See my question in the overall review comment about whether we ever give up).
- Galen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/#review176020
-----------------------------------------------------------
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
>
>
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/#review176643
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
Line 277 (original), 281 (patched)
<https://reviews.apache.org/r/59546/#comment250029>
In this case you leave "hostAddress" null.
It is used later (down around line 338) in this code:
java.net.InetSocketAddress sockAddr = new java.net.InetSocketAddress(hostAddress, portVal);
if (!locs.contains(sockAddr)) {
if (!firstUniqueLocator) {
sb.append(',');
} else {
firstUniqueLocator = false;
}
locs.add(new java.net.InetSocketAddress(hostAddress, portVal));
sb.append(locatorsb);
}
In this context a null hostAddress many any local address which is not correct.
geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
Line 291 (original), 293 (patched)
<https://reviews.apache.org/r/59546/#comment250030>
What about this UnknownHostException on the bindAddr?
I don't remember why we have a bindAddr but do you also need to allow it to be resolved later?
geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
Line 262 (original), 263 (patched)
<https://reviews.apache.org/r/59546/#comment250020>
It is unclear to me how this change helps.
We end up using these props to connect the DistributedSystem and end up calling org.apache.geode.internal.AbstractConfig.setAttribute(String, String, ConfigSource) for each one.
This method does the following for MCAST_ADDRESS:
} else if (valueType.equals(InetAddress.class)) {
try {
attObjectValue = InetAddress.getByName(attValue);
} catch (UnknownHostException ex) {
throw new IllegalArgumentException(
LocalizedStrings.AbstractConfig_0_VALUE_1_MUST_BE_A_VALID_HOST_NAME_2
.toLocalizedString(new Object[] {attName, attValue, ex.toString()}));
}
So wouldn't we still get an IllegalArgumentException here?
From your description I thought you just wanted to fix the locator list so I'm not sure why the MCAST_ADDRESS change was made. But if it is important it seems like more work is needed down in the AbstractConfig code.
- Darrel Schneider
On May 25, 2017, 12:06 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, 12:06 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/#review176113
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/admin/internal/DistributionLocatorImpl.java
Line 196 (original), 196 (patched)
<https://reviews.apache.org/r/59546/#comment249446>
Removed it. here we validate hist only
geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
Line 277 (original), 281 (patched)
<https://reviews.apache.org/r/59546/#comment249447>
Removed commented code
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
Line 1555 (original), 1555 (patched)
<https://reviews.apache.org/r/59546/#comment249448>
This is just for compare purpose.
geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
Line 131 (original), 132 (patched)
<https://reviews.apache.org/r/59546/#comment249449>
Yes, that's the change.Removed commented code
geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
Lines 234 (patched)
<https://reviews.apache.org/r/59546/#comment249453>
line 131; but then hostname will be there. What I am reading is that at this point either host/hostname will be there
geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
Lines 278 (patched)
<https://reviews.apache.org/r/59546/#comment249451>
Ip should be fine. This can throw exception when host/hostname is not appropriate.
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
Lines 346 (patched)
<https://reviews.apache.org/r/59546/#comment249452>
If locator host is still not avialable then server won't be able to join the cluster.
- Hitesh Khamesra
On May 25, 2017, 7:06 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, 7:06 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59546/
-----------------------------------------------------------
(Updated May 25, 2017, 7:06 p.m.)
Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Udo Kohlmeyer.
Changes
-------
Updated diff; removed commented code
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 (updated)
-----
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/2/
Changes: https://reviews.apache.org/r/59546/diff/1-2/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 59546: GEODE-2940 Remove verification of locator
host on start
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
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 (updated)
-------
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