[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17295567#comment-17295567
]
ASF GitHub Bot commented on GEODE-8872:
---------------------------------------
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:
us...@infra.apache.org
> Add client option, to request locators internal host addresses
> ---------------------------------------------------------------
>
> Key: GEODE-8872
> URL: https://issues.apache.org/jira/browse/GEODE-8872
> Project: Geode
> Issue Type: New Feature
> Components: client/server
> Reporter: Mario Ivanac
> Assignee: Mario Ivanac
> Priority: Major
> Labels: pull-request-available
>
> Additional option for clients, when pool is created, to request locators
> internal host addresses.
> When sending LocatorListRequest in some cases we need to get internal host
> addresses.
> To describe use case:
> When deploying geode, for locator we define hostname-for-clients. This is
> external address used for WAN, and external 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 message was sent by Atlassian Jira
(v8.3.4#803005)
