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/12/18 16:30:22 UTC
[GitHub] [geode] mivanac opened a new pull request #7214: Newfeature/geode 9484
mivanac opened a new pull request #7214:
URL: https://github.com/apache/geode/pull/7214
<!-- 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] dschneider-pivotal commented on a change in pull request #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7214:
URL: https://github.com/apache/geode/pull/7214#discussion_r799680564
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +360,32 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) throws Exception {
+ Region r1 = getCache().getRegion(REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ // ignore
+ }
+ Thread.sleep(1000);
+ }
+ }
+
+ private int getNotNullEntriesNumber(int entries) {
+ int notNullEntries = 0;
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ Object value = r1.get("" + i, "" + i);
Review comment:
this uses the Region.get(key, callbackArg) form of get. Did you really need the callback arg here or could you just call `r1.get(""+i)`?
--
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 #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
mivanac commented on a change in pull request #7214:
URL: https://github.com/apache/geode/pull/7214#discussion_r799420583
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +360,32 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) throws Exception {
+ Region r1 = getCache().getRegion(REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ // ignore
Review comment:
We will get here org.apache.geode.cache.client.ServerConnectivityException: Pool unexpected socket timed out on client...
--
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] dschneider-pivotal commented on a change in pull request #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7214:
URL: https://github.com/apache/geode/pull/7214#discussion_r799678822
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +360,32 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) throws Exception {
+ Region r1 = getCache().getRegion(REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ // ignore
Review comment:
If the client times out the put (which is why you get this exception) then the operation is still in progress on the servers. While it is still in progress because it is waiting for server2 to leave the membership view it will have already done the put on server1 but not yet distributed it to server3. You need to wait for these in progress ops to finish before doing your validation. It seems like this would still be a flaky test even with your new LIMIT feature.
--
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 #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
mivanac closed pull request #7214:
URL: https://github.com/apache/geode/pull/7214
--
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] dschneider-pivotal commented on a change in pull request #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7214:
URL: https://github.com/apache/geode/pull/7214#discussion_r798856752
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +100,63 @@ public final void postSetUp() throws Exception {
// Server2 VM
server2 = host.getVM(1);
+ // Server3 VM
+ server3 = host.getVM(2);
+
// Client 1 VM
- client1 = host.getVM(2);
+ client1 = host.getVM(3);
// client 2 VM
- client2 = host.getVM(3);
+ client2 = host.getVM(4);
- PORT1 = server1.invoke(this::createServerCache);
- PORT2 = server2.invoke(this::createServerCache);
+ PORT1 = server1.invoke(() -> createServerCache());
+ PORT2 = server2.invoke(() -> createServerCache());
+ PORT3 = server3.invoke(() -> createServerCache());
- client1.invoke(
- () -> createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1, PORT2));
- client2.invoke(
- () -> createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1, PORT2));
+ hostnameServer1 = NetworkUtils.getServerHostName(server1.getHost());
+ hostnameServer3 = NetworkUtils.getServerHostName(server3.getHost());
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() throws Exception {
+ client1.invoke(
+ () -> createClientCache(hostnameServer1, PORT1));
+ client2.invoke(
+ () -> createClientCache(hostnameServer3, PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
+
+ // Wait for some entries to be put
+ Thread.sleep(5000);
Review comment:
Instead of sleeping (we have a goal of not sleeping in tests), run a task on server1 that returns the number of entries stored on it. Then the test could wait until this task returns at least some expected number of entries.
The best practice is to use GeodeAwaitility instead of sleep and you can find lots of tests that use it for examples.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +100,63 @@ public final void postSetUp() throws Exception {
// Server2 VM
server2 = host.getVM(1);
+ // Server3 VM
+ server3 = host.getVM(2);
+
// Client 1 VM
- client1 = host.getVM(2);
+ client1 = host.getVM(3);
// client 2 VM
- client2 = host.getVM(3);
+ client2 = host.getVM(4);
- PORT1 = server1.invoke(this::createServerCache);
- PORT2 = server2.invoke(this::createServerCache);
+ PORT1 = server1.invoke(() -> createServerCache());
+ PORT2 = server2.invoke(() -> createServerCache());
+ PORT3 = server3.invoke(() -> createServerCache());
- client1.invoke(
- () -> createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1, PORT2));
- client2.invoke(
- () -> createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1, PORT2));
+ hostnameServer1 = NetworkUtils.getServerHostName(server1.getHost());
+ hostnameServer3 = NetworkUtils.getServerHostName(server3.getHost());
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() throws Exception {
Review comment:
The way this test is written, it gives the impression that it is validating the size on the clients. If you ran a task directly on server1 and server3 instead client1 and client2 I think it would be more clear. It seems like you could just call Region.size() on each server.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +360,32 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) throws Exception {
+ Region r1 = getCache().getRegion(REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ // ignore
Review comment:
It doesn't seem like you should see any exceptions from put since your client put is being sent to server1 which never goes down. So it seems like you should remove this try/catch and just throw any unexpected exception to the caller of doPuts. Do you see exceptions here?
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +360,32 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) throws Exception {
+ Region r1 = getCache().getRegion(REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ // ignore
+ }
+ Thread.sleep(1000);
Review comment:
don't sleep in tests.
--
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 #7214: Newfeature/geode 9484
Posted by GitBox <gi...@apache.org>.
mivanac closed pull request #7214:
URL: https://github.com/apache/geode/pull/7214
--
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