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 2020/06/25 11:40:59 UTC
[GitHub] [geode] alb3rtobr opened a new pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
alb3rtobr opened a new pull request #5303:
URL: https://github.com/apache/geode/pull/5303
The DistributedPingMessage is handled asynchronously: I have check that if I introduce a sleep `process()` method of `DistributedPingServer`, the test case fails because the `ClientHealthMonitor` has not received the ping.
I suspect that in the ocassions where the test failed was due to a race condition between the check of the `ClientHealthMonitor` content and the process of the message. So Im introducing a wait when checking the `ClientHealthMonitor` to avoid this.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5303:
URL: https://github.com/apache/geode/pull/5303#discussion_r445504922
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -129,7 +130,9 @@ public void testPingWrongServer() {
PingOp.execute(pool, new ServerLocation(NetworkUtils.getServerHostName(), PORT1), server2ID);
// if the ping made it to server2 it will have the client's ID in its health monitor
server2.invoke(() -> {
- assertEquals(1, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ await("For heartbeat to be received").timeout(Duration.ofMinutes(1))
Review comment:
I agree in that 5 minutes might be "too much" for this test, but it's the current standard and there are only a handful of tests (8 I think) that overrides it, so I think is better to keep using the defaults for everything new.
As a side note, the poll occurs every 100 milliseconds anyway, so the condition should be met way sooner than 5 minutes.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5303:
URL: https://github.com/apache/geode/pull/5303#discussion_r445531653
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -117,19 +116,21 @@ public void testPingWrongServer() {
InternalDistributedMember server2ID = server2.invoke("get ID", () -> cache.getMyId());
pool = (PoolImpl) createClientCache(NetworkUtils.getServerHostName(), PORT1);
// send the ping to server1 but use server2's identifier so the ping will be forwarded
-
ClientProxyMembershipID proxyID = server1.invoke(
() -> CacheClientNotifier.getInstance().getClientProxies().iterator().next().getProxyID());
logger.info("ProxyID is : " + proxyID);
Review comment:
This logging message can be removed.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -117,19 +116,21 @@ public void testPingWrongServer() {
InternalDistributedMember server2ID = server2.invoke("get ID", () -> cache.getMyId());
pool = (PoolImpl) createClientCache(NetworkUtils.getServerHostName(), PORT1);
// send the ping to server1 but use server2's identifier so the ping will be forwarded
-
ClientProxyMembershipID proxyID = server1.invoke(
() -> CacheClientNotifier.getInstance().getClientProxies().iterator().next().getProxyID());
logger.info("ProxyID is : " + proxyID);
server2.invoke(() -> {
assertThat(ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().contains(proxyID))
.isFalse();
- assertEquals(0, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ assertThat(ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size())
+ .isEqualTo(0);
});
PingOp.execute(pool, new ServerLocation(NetworkUtils.getServerHostName(), PORT1), server2ID);
// if the ping made it to server2 it will have the client's ID in its health monitor
server2.invoke(() -> {
- assertEquals(1, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ await("For heartbeat to be received").untilAsserted(() -> assertThat(
+ ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size())
+ .isEqualTo(1));
ClientProxyMembershipID proxyIDFound =
ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().iterator().next();
logger.info("ProxyID found in clientHealthMonitor: " + proxyIDFound);
Review comment:
This logging message can be removed.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5303:
URL: https://github.com/apache/geode/pull/5303#discussion_r445500467
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -129,7 +130,9 @@ public void testPingWrongServer() {
PingOp.execute(pool, new ServerLocation(NetworkUtils.getServerHostName(), PORT1), server2ID);
// if the ping made it to server2 it will have the client's ID in its health monitor
server2.invoke(() -> {
- assertEquals(1, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ await("For heartbeat to be received").timeout(Duration.ofMinutes(1))
Review comment:
Please use the default timeout (5 minutes) set by `GeodeAwaitility` instead: `await().untilAsserted()` (without the `timeout`).
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -20,6 +20,8 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
Review comment:
The current preferred approach is to always use `org.assertj.core.api.Assertions` instead of `org.junit.Assert`, can you change 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] alb3rtobr commented on a change in pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #5303:
URL: https://github.com/apache/geode/pull/5303#discussion_r445502667
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -129,7 +130,9 @@ public void testPingWrongServer() {
PingOp.execute(pool, new ServerLocation(NetworkUtils.getServerHostName(), PORT1), server2ID);
// if the ping made it to server2 it will have the client's ID in its health monitor
server2.invoke(() -> {
- assertEquals(1, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ await("For heartbeat to be received").timeout(Duration.ofMinutes(1))
Review comment:
I thought 5 minutes were too much for this case but if its ok for you, then I can change it, no problem.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #5303:
URL: https://github.com/apache/geode/pull/5303#discussion_r445550192
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -117,19 +116,21 @@ public void testPingWrongServer() {
InternalDistributedMember server2ID = server2.invoke("get ID", () -> cache.getMyId());
pool = (PoolImpl) createClientCache(NetworkUtils.getServerHostName(), PORT1);
// send the ping to server1 but use server2's identifier so the ping will be forwarded
-
ClientProxyMembershipID proxyID = server1.invoke(
() -> CacheClientNotifier.getInstance().getClientProxies().iterator().next().getProxyID());
logger.info("ProxyID is : " + proxyID);
Review comment:
done!
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscDUnitTest.java
##########
@@ -117,19 +116,21 @@ public void testPingWrongServer() {
InternalDistributedMember server2ID = server2.invoke("get ID", () -> cache.getMyId());
pool = (PoolImpl) createClientCache(NetworkUtils.getServerHostName(), PORT1);
// send the ping to server1 but use server2's identifier so the ping will be forwarded
-
ClientProxyMembershipID proxyID = server1.invoke(
() -> CacheClientNotifier.getInstance().getClientProxies().iterator().next().getProxyID());
logger.info("ProxyID is : " + proxyID);
server2.invoke(() -> {
assertThat(ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().contains(proxyID))
.isFalse();
- assertEquals(0, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ assertThat(ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size())
+ .isEqualTo(0);
});
PingOp.execute(pool, new ServerLocation(NetworkUtils.getServerHostName(), PORT1), server2ID);
// if the ping made it to server2 it will have the client's ID in its health monitor
server2.invoke(() -> {
- assertEquals(1, ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size());
+ await("For heartbeat to be received").untilAsserted(() -> assertThat(
+ ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().size())
+ .isEqualTo(1));
ClientProxyMembershipID proxyIDFound =
ClientHealthMonitor.getInstance().getClientHeartbeats().keySet().iterator().next();
logger.info("ProxyID found in clientHealthMonitor: " + proxyIDFound);
Review comment:
done!
----------------------------------------------------------------
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] jujoramos merged pull request #5303: GEODE-8176: Fix for flaky testPingWrongServer
Posted by GitBox <gi...@apache.org>.
jujoramos merged pull request #5303:
URL: https://github.com/apache/geode/pull/5303
----------------------------------------------------------------
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