You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/01/24 21:08:37 UTC
[GitHub] [geode] mivanac opened a new pull request #5948: GEODE-8872: added client impacts
mivanac opened a new pull request #5948:
URL: https://github.com/apache/geode/pull/5948
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [*] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [*] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [*] Is your initial contribution a single, squashed commit?
- [*] Does `gradlew build` run cleanly?
- [*] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850729080
> Hi,
> thanks for all comments.
>
> From received comments, I see more concerns regarding exposing internal address and security, so I will close current PR,
> and new proposal describe in RFC,
>
> Regards,
> Mario
Thanks Mario!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r586386215
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
##########
@@ -207,6 +207,13 @@ default boolean getThreadLocalConnections() {
*/
boolean getMultiuserAuthentication();
+ /**
+ * Returns true ir request locator internal address is enabled on this pool.
+ *
+ * @see PoolFactory#setRequestLocatorInternalAddress(boolean)
+ * @since Geode 1.14
+ */
+ boolean getRequestLocatorInternalAddress();
Review comment:
Thanks for comment. I will update.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-824700456
If we deploy clients in internal network (for example in the same kubernetes cluster as geode), then for those clients to connect with locator, communication will go from internal network to external, then from external to internal. This increases latency, and we should communicate over the shortest path. This is the reason that for some clients we need internal address exposed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850603137
Hi,
thanks for all comments.
From received comments, I see more concerns regarding exposing internal address and security, so I will close current PR,
and new proposal describe in RFC,
Regards,
Mario
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-766946142
Mario, this is marked as a Draft PR. Do you want early reviews before taking it out of Draft status?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r564014872
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocator.java
##########
@@ -378,24 +379,40 @@ public void endResponse(Object request, long startTime) {
}
-
- private List<ServerLocation> getLocators() {
- if (cachedLocators != null) {
+ private List<ServerLocation> getLocators(boolean requestInternal) {
+ if (cachedLocators != null && cachedRequestInternalLocators == requestInternal) {
return cachedLocators;
} else {
synchronized (cachedLocatorsLock) {
List<ControllerProfile> profiles = advisor.fetchControllers();
List<ServerLocation> result = new ArrayList<>(profiles.size() + 1);
for (ControllerProfile profile : profiles) {
- result.add(buildServerLocation(profile));
+ result.add(buildServerLocation(profile, requestInternal));
}
- result.add(new ServerLocation(hostNameForClients, port));
+ String host;
+ if (requestInternal) {
+ host = hostName;
+ } else {
+ host = hostNameForClients;
+ }
+ result.add(new ServerLocation(host, port));
+ cachedRequestInternalLocators = requestInternal;
cachedLocators = result;
return result;
}
}
}
+ protected static ServerLocation buildServerLocation(GridProfile p, boolean requestInternal) {
+ String host;
+ if (requestInternal) {
+ host = p.getInternalHost();
+ } else {
+ host = p.getHost();
+ }
+ return new ServerLocation(host, p.getPort());
+ }
+
protected static ServerLocation buildServerLocation(GridProfile p) {
Review comment:
Could uses of this method be replaced with calls to the new, two-argument method instead, to reduce redundancy?
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java
##########
@@ -767,6 +778,10 @@ public void sameAs(Object obj) {
throw new RuntimeException(
String.format("Pool %s are different", "servers"));
}
+ if (getRequestLocatorInternalAddress() != other.getRequestLocatorInternalAddress()) {
+ throw new RuntimeException(
+ String.format("Pool %s is different", "requestLocatorInternalAddress"));
Review comment:
This doesn't need to be a `String.format()` call, it could just be `"Pool requestLocatorInternalAddress is different"`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-766953659
Comments are appreciated in any PR status.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-766953659
Comments are appreciated in any PR status.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850547011
Tagging @kirklund for a re-review or to continue the discussion, since his concerns may have been addressed at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850589885
@mivanac
>> I have explained use case for this PR. If community is not interested in such change, I will close this PR.
Its not community is interested or not; it is the nature of the problem that could raise many questions/concerns and its effectiveness that could bring different views and could take time to resolve (for good). If you can see many community members are involved here trying to get to an optimal conclusion.
Looking at the problem and the solution proposed; this problem seems to be a good candidate to raise an RFC, where discussion could have been carried out to understand the pros and cons of the issue and solutions.
From the problem description it seems you are trying to address performance issue with client connection. It would have been nice to see, what kind of performance impact we are looking at (e.g. with prototype)
Because of the sensitive nature of the issue (use/exposure of a n/w info) it will be nice to talked about any security issues that this could bring about.
As I mentioned above; some of the areas that could have been discussed in RFC are:
- Benefits (with some sample perf number)
- Security concerns/issues with this
- Case where an external client tries to fetch the internal info (outside of k8 env - what happens in this case)
- How other client connection could be impacted or can use similar in future (Java, C++, JMX, Gfsh, pulse connections)
- Requirement for Backward compatibility, rolling upgrade tests (I am not seeing any here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac closed pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac closed pull request #5948:
URL: https://github.com/apache/geode/pull/5948
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-857625358
RFC created for this improvement https://cwiki.apache.org/confluence/display/GEODE/Get+locators+internal+host+addresses+for+clients+in+same+subnet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-789818612
Updated code. Working on additional test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r585940678
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
##########
@@ -207,6 +207,13 @@ default boolean getThreadLocalConnections() {
*/
boolean getMultiuserAuthentication();
+ /**
+ * Returns true ir request locator internal address is enabled on this pool.
+ *
+ * @see PoolFactory#setRequestLocatorInternalAddress(boolean)
+ * @since Geode 1.14
+ */
+ boolean getRequestLocatorInternalAddress();
Review comment:
This method name is a little confusing, as it might be mistaken for a getter for the locator internal address itself. It might be better as "isRequestLocatorInternalAddressEnabled", and the corresponding setter as "setRequestLocatorInternalAddressEnabled".
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -257,6 +257,9 @@ public Set adviseProfileRemove() {
private String host;
+ /** valid only for ControllerProfile */
+ private String internalHost;
Review comment:
It doesn't seem correct to have a field in a parent class that only belongs to one of its child classes. Can this be moved into ControllerProfile?
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/PoolFactory.java
##########
@@ -220,6 +220,14 @@
* @since GemFire 6.5
*/
boolean DEFAULT_MULTIUSER_AUTHENTICATION = false;
+ /**
+ * The default value for whether to request locator internal address.
Review comment:
Slightly more detail would be good here, explaining when exactly the locator internal address would be requested if this was set to true.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
##########
@@ -207,6 +207,13 @@ default boolean getThreadLocalConnections() {
*/
boolean getMultiuserAuthentication();
+ /**
+ * Returns true ir request locator internal address is enabled on this pool.
Review comment:
Typo here, should be "Returns true if..."
##########
File path: geode-dunit/src/main/java/org/apache/geode/cache/client/internal/LocatorTestBase.java
##########
@@ -141,6 +141,34 @@ protected int startLocator(final String hostName, final String otherLocators) th
return locator.getPort();
}
+ protected int startLocator(final String hostName, final String otherLocators,
Review comment:
This method is never used.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -432,6 +447,17 @@ public boolean equals(Object obj) {
if (this.gp.getPort() == other.gp.getPort()) {
final String thisHost = this.gp.getHost();
final String otherHost = other.gp.getHost();
+ final String thisInternalHost = this.gp.getInternalHost();
Review comment:
If internalHost is being included in `equals()` calculations, it should probably also be included in hashCode()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850220922
I have explained use case for this PR. If community is not interested in such change, I will close this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp edited a comment on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mhansonp edited a comment on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-830259463
Hi, I would really appreciate it if you made the effort to clear our the warnings that are in all of the tests you touch. That will leave us cleaner code and a more clear understanding of our real problems (e.g. deprecated code). It is not required, but it should be something we try to do. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-830259463
Hi, I would really appreciate it if you made the effort to clear our the warnings that are in all of the tests you touch. That will leave us cleaner code and a more clear understanding of our real problems (e.g. deprecated code). It is not required for this PR, but it should be something we try to do. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade edited a comment on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
agingade edited a comment on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850589885
@mivanac
>> I have explained use case for this PR. If community is not interested in such change, I will close this PR.
Its not community is interested or not; it is the nature of the problem that could raise many questions/concerns and its effectiveness that could bring different views and could take time to resolve (for good). If you can see many community members are involved here trying to get to an optimal conclusion.
Looking at the problem and the solution proposed; this problem seems to be a good candidate to raise an RFC, where discussion could have been carried out to understand the pros and cons of the issue and solutions.
From the problem description it seems you are trying to address performance issue with client connection. It would have been nice to see, what kind of performance impact we are looking at (e.g. with prototype)
Because of the sensitive nature of the issue (use/exposure of a n/w info) it will be nice to talked about any security issues that this could bring about.
As I mentioned above; some of the areas that could have been discussed in RFC are:
- Benefits (with some sample perf number)
- Security concerns/issues with this
- Case where an external client tries to fetch the internal info (outside of k8 env - what happens in this case)
- How other client connection could be impacted or can use similar in future (Java, C++, JMX, Gfsh, pulse connections)
- Requirement for Backward compatibility, rolling upgrade tests (I am not seeing any here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-850623771
Thanks @mivanac for your help, support and understanding...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-783284073
When deploying geode, for locator we define hostname-for-clients. This is extern address used for WAN, and extern clients.
If we also deploy some clients in internal network (for example in the same kubernetes cluster as geode), then for those clients to connect with locator, communication will go from internal network to external, then from external to internal. This increases latency, and we should communicate over the shortest path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r564014872
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocator.java
##########
@@ -378,24 +379,40 @@ public void endResponse(Object request, long startTime) {
}
-
- private List<ServerLocation> getLocators() {
- if (cachedLocators != null) {
+ private List<ServerLocation> getLocators(boolean requestInternal) {
+ if (cachedLocators != null && cachedRequestInternalLocators == requestInternal) {
return cachedLocators;
} else {
synchronized (cachedLocatorsLock) {
List<ControllerProfile> profiles = advisor.fetchControllers();
List<ServerLocation> result = new ArrayList<>(profiles.size() + 1);
for (ControllerProfile profile : profiles) {
- result.add(buildServerLocation(profile));
+ result.add(buildServerLocation(profile, requestInternal));
}
- result.add(new ServerLocation(hostNameForClients, port));
+ String host;
+ if (requestInternal) {
+ host = hostName;
+ } else {
+ host = hostNameForClients;
+ }
+ result.add(new ServerLocation(host, port));
+ cachedRequestInternalLocators = requestInternal;
cachedLocators = result;
return result;
}
}
}
+ protected static ServerLocation buildServerLocation(GridProfile p, boolean requestInternal) {
+ String host;
+ if (requestInternal) {
+ host = p.getInternalHost();
+ } else {
+ host = p.getHost();
+ }
+ return new ServerLocation(host, p.getPort());
+ }
+
protected static ServerLocation buildServerLocation(GridProfile p) {
Review comment:
Could uses of this method be replaced with calls to the new, two-argument method instead, to reduce redundancy?
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java
##########
@@ -767,6 +778,10 @@ public void sameAs(Object obj) {
throw new RuntimeException(
String.format("Pool %s are different", "servers"));
}
+ if (getRequestLocatorInternalAddress() != other.getRequestLocatorInternalAddress()) {
+ throw new RuntimeException(
+ String.format("Pool %s is different", "requestLocatorInternalAddress"));
Review comment:
This doesn't need to be a `String.format()` call, it could just be `"Pool requestLocatorInternalAddress is different"`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #5948:
URL: https://github.com/apache/geode/pull/5948#issuecomment-766946142
Mario, this is marked as a Draft PR. Do you want early reviews before taking it out of Draft status?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5948: GEODE-8872: added client impacts
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r587691115
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/AutoConnectionSourceDUnitTest.java
##########
@@ -453,6 +454,80 @@ public void testClientMembershipListener() {
Assert.assertEquals(0, serverListener.getJoins());
}
+
+ @Test
+ public void testClientGetsLocatorListwithExternalAddress() throws Exception {
Review comment:
Typo here, this should be "ListWithExternal"
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/AutoConnectionSourceDUnitTest.java
##########
@@ -64,6 +64,7 @@
@Override
public final void postSetUp() {
addIgnoredException("NoAvailableLocatorsException");
+ addIgnoredException("SocketException");
Review comment:
Is this IgnoredException needed? When I comment out this line, all the tests in the class still pass. Also, if it's required for only one test case, it should be added in only that test case, to avoid masking failures in other tests.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/client/ClientCacheFactoryJUnitTest.java
##########
@@ -460,4 +460,22 @@ public void configuringPdxDiskStoreThroughXMLShouldLogWarningMessage() throws IO
.contains("PDX persistence is not supported on client side.")).isTrue();
}
}
+
+ @Test
+ public void testDefaultPoolRequestLocatorInternalAddressEnabled_defaultvalue() throws Exception {
+ clientCache = new ClientCacheFactory().setRequestLocatorInternalAddressEnabled(false)
+ .addPoolServer(InetAddress.getLocalHost().getHostName(), 7777).create();
Review comment:
Is there a reason this port number is being used? Might it be better to use `AvailablePortHelper.getRandomAvailableTCPPort()` here to prevent possible port collisions?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/client/ClientCacheFactoryJUnitTest.java
##########
@@ -460,4 +460,22 @@ public void configuringPdxDiskStoreThroughXMLShouldLogWarningMessage() throws IO
.contains("PDX persistence is not supported on client side.")).isTrue();
}
}
+
+ @Test
+ public void testDefaultPoolRequestLocatorInternalAddressEnabled_defaultvalue() throws Exception {
+ clientCache = new ClientCacheFactory().setRequestLocatorInternalAddressEnabled(false)
+ .addPoolServer(InetAddress.getLocalHost().getHostName(), 7777).create();
+ Pool defaultPool = clientCache.getDefaultPool();
+ assertThat(defaultPool.isRequestLocatorInternalAddressEnabled()).isFalse();
+ }
+
+ @Test
+ public void testDefaultPoolRequestLocatorInternalAddressEnabled() throws Exception {
Review comment:
This test name could be more descriptive. Maybe something like "defaultPoolUsesValueOfRequestLocatorInternalAddressEnabledSetInClientCacheFactory"
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/AutoConnectionSourceDUnitTest.java
##########
@@ -453,6 +454,80 @@ public void testClientMembershipListener() {
Assert.assertEquals(0, serverListener.getJoins());
}
+
+ @Test
+ public void testClientGetsLocatorListwithExternalAddress() throws Exception {
+ final String hostName = getServerHostName();
+ VM locator0VM = VM.getVM(0);
+ VM locator1VM = VM.getVM(1);
+
+ final int locator0Port =
+ locator0VM.invoke("Start Locator1 ", () -> startLocator(hostName, "", "127.0.0.1"));
+ final int locator1Port = locator1VM.invoke("Start Locator2 ",
+ () -> startLocator(hostName, getLocatorString(hostName, locator0Port), "127.0.0.1"));
+ assertThat(locator0Port).isGreaterThan(0);
+ assertThat(locator1Port).isGreaterThan(0);
+
+ startBridgeClient(null, hostName, locator0Port, false);
+ InetSocketAddress locatorToWaitFor = new InetSocketAddress("127.0.0.1", locator1Port);
+ MyLocatorCallback callback = (MyLocatorCallback) remoteObjects.get(CALLBACK_KEY);
+
+ boolean discovered = callback.waitForDiscovery(locatorToWaitFor, MAX_WAIT);
+ Assert.assertTrue(
+ "Waited " + MAX_WAIT + " for " + locatorToWaitFor
+ + " to be discovered on client. List is now: " + callback.getDiscovered(),
+ discovered);
+
+ InetSocketAddress[] initialLocators =
+ new InetSocketAddress[] {new InetSocketAddress(hostName, locator0Port)};
+
+ InetSocketAddress[] expectedLocators =
+ new InetSocketAddress[] {new InetSocketAddress("127.0.0.1", locator0Port),
+ new InetSocketAddress("127.0.0.1", locator1Port)};
+
+ final Pool pool = PoolManager.find(POOL_NAME);
+
+ verifyLocatorsMatched(initialLocators, pool.getLocators());
+ verifyLocatorsMatched(expectedLocators, pool.getOnlineLocators());
+ }
+
+ @Test
+ public void testClientGetsLocatorListwithInternalAddress() throws Exception {
Review comment:
Typo here, should be "ListWithInternal"
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/client/internal/AutoConnectionSourceDUnitTest.java
##########
@@ -453,6 +454,80 @@ public void testClientMembershipListener() {
Assert.assertEquals(0, serverListener.getJoins());
}
+
+ @Test
+ public void testClientGetsLocatorListwithExternalAddress() throws Exception {
+ final String hostName = getServerHostName();
+ VM locator0VM = VM.getVM(0);
+ VM locator1VM = VM.getVM(1);
+
+ final int locator0Port =
+ locator0VM.invoke("Start Locator1 ", () -> startLocator(hostName, "", "127.0.0.1"));
Review comment:
If possible could the `hostNameForClients` arguments be different for each locator, and extracted to variables, so that it's clearer when they're being compared later?
##########
File path: geode-core/src/test/java/org/apache/geode/cache/client/internal/PoolImplTest.java
##########
@@ -137,6 +158,8 @@ private PoolImpl getPool(int retryAttemptsAttribute) {
doReturn(1).when(poolAttributes).getMaxConnections();
doReturn((long) 10e8).when(poolAttributes).getPingInterval();
doReturn(retryAttemptsAttribute).when(poolAttributes).getRetryAttempts();
+ doReturn(requestLocatorInternalAddressEnabled).when(poolAttributes)
Review comment:
The comment above this change states that these mocks are required for pool validation and setup to complete and that "The values of these attributes have no importance to the assertions of the test itself." In the case of this line, that's not true though, as the value of `requestLocatorInternalAddressEnabled` returned is explicitly what's being tested in the added test cases, and the pool can still be succesfully created with this line commented out. It might be better to change the signature of the `getPool()` method to take a `PoolAttributes` argument which can have the required additional mocking added in the individual test cases that require it, and leave `getPool()` to only do the mocking that's necessary to create a pool. I believe the same is true of the `retryAttemptsAttribute` argument, so that could also be removed from `getPool()` and the necessary mocking done only in the test methods that require it.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/cache/client/ClientCacheFactoryJUnitTest.java
##########
@@ -460,4 +460,22 @@ public void configuringPdxDiskStoreThroughXMLShouldLogWarningMessage() throws IO
.contains("PDX persistence is not supported on client side.")).isTrue();
}
}
+
+ @Test
+ public void testDefaultPoolRequestLocatorInternalAddressEnabled_defaultvalue() throws Exception {
Review comment:
This test name is misleading, since the value of `requestLocatorInternalAddressEnabled` is being explicitly set to false, so the default value isn't being tested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org