[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17353845#comment-17353845 ] ASF GitHub Bot commented on GEODE-8872: --- mivanac closed pull request #736: URL: https://github.com/apache/geode-native/pull/736 -- 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17337578#comment-17337578 ] ASF GitHub Bot commented on GEODE-8872: --- pdxcodemonkey commented on pull request #736: URL: https://github.com/apache/geode-native/pull/736#issuecomment-830317968 Should we close or convert to draft again, then? I'd like to get to a state where we don't have PRs hanging around for weeks and weeks... -- 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17337575#comment-17337575 ] ASF GitHub Bot commented on GEODE-8872: --- mivanac commented on pull request #736: URL: https://github.com/apache/geode-native/pull/736#issuecomment-830292508 This PR is currently paused, since PR for java (server) impacts is currently blocked. -- 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17337570#comment-17337570 ] ASF GitHub Bot commented on GEODE-8872: --- pdxcodemonkey commented on pull request #736: URL: https://github.com/apache/geode-native/pull/736#issuecomment-830288602 @mivanac what's the current status of this one? Looks like Jake's last concern still needs to be addressed, and CI jobs haven't been run. Pushing an empty commit should wake up CI... -- 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17337568#comment-17337568 ] ASF GitHub Bot commented on GEODE-8872: --- pdxcodemonkey commented on a change in pull request #736: URL: https://github.com/apache/geode-native/pull/736#discussion_r624095113 ## File path: cppcache/src/ThinClientLocatorHelper.cpp ## @@ -123,7 +124,8 @@ std::shared_ptr ThinClientLocatorHelper::sendRequest( auto conn = createConnection(location); auto data = m_poolDM->getConnectionManager().getCacheImpl()->createDataOutput(); -data.writeInt(static_cast(1001)); // GOSSIPVERSION +data.writeInt(static_cast(1002)); // GOSSIPVERSION +data.writeInt(static_cast(125)); // GEODE 1.14.0 Review comment: Reolving - this change has been removed, and the code on `develop` uses `Version::getOrdinal()` as expected. -- 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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(),
).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(),
).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);
+
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17294621#comment-17294621 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17294507#comment-17294507
]
ASF GitHub Bot commented on GEODE-8872:
---
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:
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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17294031#comment-17294031
]
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_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:
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)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17288311#comment-17288311 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17276520#comment-17276520
]
ASF GitHub Bot commented on GEODE-8872:
---
pivotal-jbarrett commented on a change in pull request #736:
URL: https://github.com/apache/geode-native/pull/736#discussion_r568006417
##
File path: cppcache/src/ThinClientLocatorHelper.cpp
##
@@ -123,7 +124,8 @@ std::shared_ptr
ThinClientLocatorHelper::sendRequest(
auto conn = createConnection(location);
auto data =
m_poolDM->getConnectionManager().getCacheImpl()->createDataOutput();
-data.writeInt(static_cast(1001)); // GOSSIPVERSION
+data.writeInt(static_cast(1002)); // GOSSIPVERSION
Review comment:
Let's move this GOSSIPVERSION to a constant somewhere
##
File path: clicache/src/PoolFactory.cpp
##
@@ -425,6 +425,26 @@ namespace Apache
}
+ PoolFactory^ PoolFactory::SetRequestLocatorInternalAddress( bool
requestInternal )
+ {
+ _GF_MG_EXCEPTION_TRY2/* due to auto replace */
Review comment:
C++/CLI formatting looks a bit off.
##
File path: cppcache/src/ThinClientLocatorHelper.cpp
##
@@ -123,7 +124,8 @@ std::shared_ptr
ThinClientLocatorHelper::sendRequest(
auto conn = createConnection(location);
auto data =
m_poolDM->getConnectionManager().getCacheImpl()->createDataOutput();
-data.writeInt(static_cast(1001)); // GOSSIPVERSION
+data.writeInt(static_cast(1002)); // GOSSIPVERSION
+data.writeInt(static_cast(125)); // GEODE 1.14.0
Review comment:
Use `Version::getOrdinal()`. You will need to update the ordinal to
1.14.0. This also means any protocol changes from 1.0.0, the current value, to
1.14.0 need to be implemented.
##
File path: cppcache/src/LocatorListRequest.hpp
##
@@ -34,13 +34,16 @@ class Serializable;
class LocatorListRequest : public ServerLocationRequest {
private:
std::string m_servergroup;
+ bool m_requestInternalAddress;
public:
- explicit LocatorListRequest(const std::string& servergroup = "");
+ explicit LocatorListRequest(const std::string& servergroup = "",
Review comment:
I suspect this signature has some really strange undefined behavior and
now might be a time to fix it. We should probably go a step further and fix the
default value assigned to a `std::string` reference. I think the keyword
`explicit` can be dropped now since there is more than one argument. Maybe we
default the first argument can convert but.. 路 What does clang-tidy say?
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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17275718#comment-17275718 ] ASF GitHub Bot commented on GEODE-8872: --- mivanac opened a new pull request #736: URL: https://github.com/apache/geode-native/pull/736 Native client impacts for ticket GEODE-8872 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271839#comment-17271839 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271815#comment-17271815
]
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_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 getLocators() {
-if (cachedLocators != null) {
+ private List getLocators(boolean requestInternal) {
+if (cachedLocators != null && cachedRequestInternalLocators ==
requestInternal) {
return cachedLocators;
} else {
synchronized (cachedLocatorsLock) {
List profiles = advisor.fetchControllers();
List 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:
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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271813#comment-17271813 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271797#comment-17271797 ] ASF GitHub Bot commented on GEODE-8872: --- 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 d...@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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[
https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271658#comment-17271658
]
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_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 getLocators() {
-if (cachedLocators != null) {
+ private List getLocators(boolean requestInternal) {
+if (cachedLocators != null && cachedRequestInternalLocators ==
requestInternal) {
return cachedLocators;
} else {
synchronized (cachedLocatorsLock) {
List profiles = advisor.fetchControllers();
List 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:
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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271525#comment-17271525 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17271513#comment-17271513 ] ASF GitHub Bot commented on GEODE-8872: --- 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: 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (GEODE-8872) Add client option, to request locators internal host addresses
[ https://issues.apache.org/jira/browse/GEODE-8872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17270986#comment-17270986 ] ASF GitHub Bot commented on GEODE-8872: --- 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 d...@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: 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 > > 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. -- This message was sent by Atlassian Jira (v8.3.4#803005)
