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 2022/02/07 22:09:41 UTC
[GitHub] [geode] mivanac opened a new pull request #7347: GEODE-10020: added logic for offset of initial delay between Pinger t…
mivanac opened a new pull request #7347:
URL: https://github.com/apache/geode/pull/7347
…asks
<!-- 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r837734165
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java
##########
@@ -71,6 +72,29 @@ Connection borrowConnection(ServerLocation server, long acquireTimeout,
boolean onlyUseExistingCnx)
throws AllConnectionsInUseException, NoAvailableServersException;
+ /**
+ * Borrow an existing idle connection or create a new one to a specific server. Fails after one
+ * failed attempt to create a new connection. May cause pool to exceed maxConnections by one, if
+ * no connection is available.
+ *
+ * @param serverLocationAndMemberId The server the connection needs to be to.
+ * @param acquireTimeout The amount of time to wait for a connection to become available, if
+ * onlyUseExistingCnx is set to true.
+ * @param onlyUseExistingCnx if true, will not create a new connection if none are available.
+ * @return A connection to use.
+ * @throws AllConnectionsInUseException If there is no available connection on the desired server,
+ * and onlyUseExistingCnx is set.
+ * @throws NoAvailableServersException If we can't connect to any server
+ * @throws ServerConnectivityException If finding a connection and creating a connection both fail
+ * to return a connection
+ *
+ */
+ Connection borrowConnection(ServerLocationAndMemberId serverLocationAndMemberId,
+ long acquireTimeout,
+ boolean onlyUseExistingCnx)
+ throws AllConnectionsInUseException, NoAvailableServersException;
+
+
Review comment:
updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7347: GEODE-10020: added logic for offset of initial delay between Pinger t…
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1036520885
This pull request **introduces 1 alert** when merging e24a02b6c84a2b10ae135ec55031a6cad84b310f into 21215d8ec0f236b01ccbc661a049fa53b01950cc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-cb5647a8c9611142fbfbaa74687dbfc2cef29f7d)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7347: GEODE-10020: added logic for offset of initial delay between Pinger t…
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1032305014
This pull request **introduces 1 alert** when merging 7bf1e5763c6f7faee9626063dbaae72e091e552f into 551d7d78548f10218146b074df9b0c4adc0940f7 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-4da7c87a586aee29d7127653225f15988a4ed22f)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r837732986
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionConnector.java
##########
@@ -88,6 +89,32 @@ public ConnectionImpl connectClientToServer(ServerLocation location, boolean for
}
}
+
+ public ConnectionImpl connectClientToServer(ServerLocationAndMemberId serverLocationAndMemberId,
Review comment:
updated
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java
##########
@@ -146,6 +147,46 @@ public Connection createClientToServerConnection(ServerLocation location, boolea
return connection;
}
+
+ @Override
+ public Connection createClientToServerConnection(
Review comment:
updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7347: GEODE-10020: added logic for offset of initial delay between Pinger t…
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1032014646
This pull request **introduces 1 alert** when merging 0e272bc99b341063bcbd4e39ce1e952af1662698 into 507565e994c14aa5a26f497604a36a1592ea4f28 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-97551d280dc4ae1702f5d2789bc30aa508f24963)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1083009845
This PR will be divided in 2 parts, to simplify review process.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r836644349
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/wan/SeveralGatewayReceiversWithSamePortAndHostnameForSendersTest.java
##########
@@ -215,6 +216,55 @@ public void testTwoSendersWithSameIdShouldUseSameValueForEnforceThreadsConnectTo
}
+
+
+ /**
+ * The aim of this test is verify that when several gateway receivers in a remote site share the
+ * same port and hostname-for-senders, the pings sent from the gateway senders reach the right
+ * gateway receiver and not just any of the receivers. Failure to do this may result in the
+ * closing of connections by a gateway receiver for not having received the ping in time.
+ */
+ @Test
+ public void testPingsToReceiversWithSamePortAndHostnameForSendersReachTheRightReceiver()
Review comment:
The description of this test case is exactly the same as the one of a test above and the name only changes by the final s.
Also the contents are the same with the exception of the last check.
I propose that the check of this test is moved to the first test case and that the description of that test is updated with what is intended by the check in this test.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionImpl.java
##########
@@ -117,6 +118,47 @@ public ServerQueueStatus connect(EndpointManager endpointManager, ServerLocation
return status;
}
+ public ServerQueueStatus connect(EndpointManager endpointManager,
Review comment:
There is a lot of code repeated in this method from the previous method. I suggest it to be refactored in order to avoid it.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java
##########
@@ -146,6 +147,46 @@ public Connection createClientToServerConnection(ServerLocation location, boolea
return connection;
}
+
+ @Override
+ public Connection createClientToServerConnection(
Review comment:
There is a lot of code repeated in this method from the previous method. I suggest it to be refactored in order to avoid it.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java
##########
@@ -71,6 +72,29 @@ Connection borrowConnection(ServerLocation server, long acquireTimeout,
boolean onlyUseExistingCnx)
throws AllConnectionsInUseException, NoAvailableServersException;
+ /**
+ * Borrow an existing idle connection or create a new one to a specific server. Fails after one
+ * failed attempt to create a new connection. May cause pool to exceed maxConnections by one, if
+ * no connection is available.
+ *
+ * @param serverLocationAndMemberId The server the connection needs to be to.
+ * @param acquireTimeout The amount of time to wait for a connection to become available, if
+ * onlyUseExistingCnx is set to true.
+ * @param onlyUseExistingCnx if true, will not create a new connection if none are available.
+ * @return A connection to use.
+ * @throws AllConnectionsInUseException If there is no available connection on the desired server,
+ * and onlyUseExistingCnx is set.
+ * @throws NoAvailableServersException If we can't connect to any server
+ * @throws ServerConnectivityException If finding a connection and creating a connection both fail
+ * to return a connection
+ *
+ */
+ Connection borrowConnection(ServerLocationAndMemberId serverLocationAndMemberId,
+ long acquireTimeout,
+ boolean onlyUseExistingCnx)
+ throws AllConnectionsInUseException, NoAvailableServersException;
+
+
Review comment:
What does this method do differently compared to the one passing a server? Do we really need it if it does the same thing?
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionConnector.java
##########
@@ -88,6 +89,32 @@ public ConnectionImpl connectClientToServer(ServerLocation location, boolean for
}
}
+
+ public ConnectionImpl connectClientToServer(ServerLocationAndMemberId serverLocationAndMemberId,
Review comment:
There is a lot of code repeated in this method. I suggest that you refactor it in order to avoid it.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java
##########
@@ -355,6 +385,54 @@ public Connection borrowConnection(ServerLocation server, long acquireTimeout,
throw new AllConnectionsInUseException();
}
+
+ @Override
+ public Connection borrowConnection(ServerLocationAndMemberId serverLocationAndMemberId,
Review comment:
There is a lot of code repeated in this method from the previous method. I suggest it to be refactored in order to avoid it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1073664190
Hi @Bill , @gesterzhou , @nabarunnag , @boglesby , @echobravopapa and @kamilla1201.
Could you review this PR.
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r837733301
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionImpl.java
##########
@@ -117,6 +118,47 @@ public ServerQueueStatus connect(EndpointManager endpointManager, ServerLocation
return status;
}
+ public ServerQueueStatus connect(EndpointManager endpointManager,
Review comment:
updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r837734313
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java
##########
@@ -355,6 +385,54 @@ public Connection borrowConnection(ServerLocation server, long acquireTimeout,
throw new AllConnectionsInUseException();
}
+
+ @Override
+ public Connection borrowConnection(ServerLocationAndMemberId serverLocationAndMemberId,
Review comment:
updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #7347: GEODE-10020: added logic for offset of initial delay between Pinger t…
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7347:
URL: https://github.com/apache/geode/pull/7347#issuecomment-1037083146
This pull request **introduces 1 alert** when merging af97b1419a065a19fc7a6b4c8e9a16c928a030ae into a98197b5d0a3a2547e0581e475dcabaa82e6e92f - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-7b1153ec45cdcfa75077a11553a46caaa2ea174d)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac closed pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac closed pull request #7347:
URL: https://github.com/apache/geode/pull/7347
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a change in pull request #7347: GEODE-10020: improve Pinger task
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7347:
URL: https://github.com/apache/geode/pull/7347#discussion_r837732797
##########
File path: geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/wan/SeveralGatewayReceiversWithSamePortAndHostnameForSendersTest.java
##########
@@ -215,6 +216,55 @@ public void testTwoSendersWithSameIdShouldUseSameValueForEnforceThreadsConnectTo
}
+
+
+ /**
+ * The aim of this test is verify that when several gateway receivers in a remote site share the
+ * same port and hostname-for-senders, the pings sent from the gateway senders reach the right
+ * gateway receiver and not just any of the receivers. Failure to do this may result in the
+ * closing of connections by a gateway receiver for not having received the ping in time.
+ */
+ @Test
+ public void testPingsToReceiversWithSamePortAndHostnameForSendersReachTheRightReceiver()
Review comment:
Thanks for comments. This test is different. It uses 1 dispatcher thread, and it needs to prove that only one endpoint will be pinged. Previously this was not the case. I will update comment.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org