kirklund commented on a change in pull request #6739:
URL: https://github.com/apache/geode/pull/6739#discussion_r705613483
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -248,6 +293,20 @@ private void createClientCache(String host, Integer port1,
Integer port2) throws
.addCacheListener(new
EventTrackingCacheListener()).create(REGION_NAME);
}
+ private void createClientCache(String host, Integer port1) {
+ ClientCache cache;
+ Properties props = new Properties();
+ props.setProperty(MCAST_PORT, "0");
Review comment:
This is ok, but the default is `"0"` so you could skip setting
`MCAST_PORT`.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +364,40 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) {
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ e.printStackTrace();
Review comment:
Remove all catch-blocks like this and just let the exceptions propagate
out. JUnit will catch them and print good failure messages most of the time.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -248,6 +293,20 @@ private void createClientCache(String host, Integer port1,
Integer port2) throws
.addCacheListener(new
EventTrackingCacheListener()).create(REGION_NAME);
}
+ private void createClientCache(String host, Integer port1) {
+ ClientCache cache;
Review comment:
I recommend declaring variables like this on the same line that actually
sets it. One less line, and it's easier to read and understand.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +364,40 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) {
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
Review comment:
This is ok, but you don't actually need the leading `Region.SEPARATOR`.
`getCache().getRegion(REGION_NAME)` works fine.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +96,70 @@ 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(() -> 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));
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1));
+ client2.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server3.getHost()), PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
+
+ // Wait for some entries to be put
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
Review comment:
It would be better to write an `await()` call that completes when an
expected number of entries is in the region. Something like:
```
await().until(() -> getCache().getRegion(REGION_NAME).size() == 5);
```
Or even better is to make an assertion so that it'll print out the actual
size if it does timeout:
```
await().untilAsserted(() ->
assertThat(getCache().getRegion(REGION_NAME).size()).isEqualTo(5));
```
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +96,70 @@ 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(() -> 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));
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1));
+ client2.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server3.getHost()), PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
Review comment:
You should add a type and name the `AsyncInvocation` variable to be what
it's doing. So something like:
```
AsyncInvocation<Void> doPutsAsync = client1.invokeAsync(() ->
doPuts(entries));
```
Or if you're using a lot of `AsyncInvocations`, it can be helpful to include
the VM or a name on the end like `doPutsAsyncInVM1` or `doPutsAsyncInClient1`.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +96,70 @@ 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(() -> 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));
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1));
+ client2.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server3.getHost()), PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
+
+ // Wait for some entries to be put
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ // Simulate crash
+ server2.invoke(() -> {
+ MembershipManagerHelper.crashDistributedSystem(getSystemStatic());
+ });
+
+
+ try {
+ invocation.await();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ int notNullEntriesIn1 = client1.invoke(() ->
getNotNullEntriesNumber(entries));
+ int notNullEntriesIn3 = client2.invoke(() ->
getNotNullEntriesNumber(entries));
+ assertThat(notNullEntriesIn3).isEqualTo(notNullEntriesIn1);
+ }
+
/**
* This tests whether the updates are received by other clients or not , if
there are situation of
* Interest List fail over
*/
@Test
public void updatesAreProgegatedAfterFailover() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1,
PORT2));
Review comment:
You might want to create a field called `hostName` and set it in
`setUp()` by calling `getServerHostName` just once. Or could even hardcode
`localhost` which will always work.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +96,70 @@ 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(() -> 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));
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1));
+ client2.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server3.getHost()), PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
+
+ // Wait for some entries to be put
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ // Simulate crash
+ server2.invoke(() -> {
+ MembershipManagerHelper.crashDistributedSystem(getSystemStatic());
+ });
+
+
+ try {
+ invocation.await();
+ } catch (InterruptedException e) {
+ e.printStackTrace();
Review comment:
Please delete all catch-blocks like this and just add the exception to a
`throws` clause on the method.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +364,40 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) {
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
Review comment:
Please delete all catch-blocks like this and just add the exception to a
`throws` clause on the method.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -305,6 +364,40 @@ private void verifyUpdates() {
});
}
+ private void doPuts(int entries) {
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ r1.put("" + i, "" + i);
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
+ }
+
+ private int getNotNullEntriesNumber(int entries) {
+ int notNullEntries = 0;
+ Region r1 = getCache().getRegion(SEPARATOR + REGION_NAME);
+ assertNotNull(r1);
+ for (int i = 0; i < entries; i++) {
+ try {
+ Object value = r1.get("" + i, "" + i);
+ if (value != null) {
+ notNullEntries++;
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
Review comment:
Please delete all catch-blocks like this and just add the exception to a
`throws` clause on the method.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java
##########
@@ -91,30 +96,70 @@ 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(() -> 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));
IgnoredException.addIgnoredException("java.net.SocketException");
IgnoredException.addIgnoredException("Unexpected IOException");
}
+
+
+ @Test
+ public void updatesArePropagatedToAllMembersWhenOneKilled() {
+ client1.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server1.getHost()), PORT1));
+ client2.invoke(
+ () ->
createClientCache(NetworkUtils.getServerHostName(server3.getHost()), PORT3));
+ int entries = 20;
+ AsyncInvocation invocation = client1.invokeAsync(() -> doPuts(entries));
+
+ // Wait for some entries to be put
+ try {
+ Thread.sleep(5000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+
+ // Simulate crash
+ server2.invoke(() -> {
+ MembershipManagerHelper.crashDistributedSystem(getSystemStatic());
Review comment:
This is ok but you could also use `getCache().getSystem()`.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/event/DistributedEventTracker.java
##########
@@ -265,7 +272,13 @@ public void recordEvent(InternalCacheEvent event) {
canonicalizeIDs(tag, v);
}
- EventSequenceNumberHolder newEvh = new
EventSequenceNumberHolder(eventID.getSequenceID(), tag);
+ Object key = null;
+ if (event instanceof EntryEventImpl) {
+ key = ((EntryEventImpl) event).getKey();
+ }
+
+ EventSequenceNumberHolder newEvh =
Review comment:
Please avoid abbreviations and spell out a full variable name that means
something like `eventSequence` or `eventSequenceId`.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/event/EventSequenceNumberHolder.java
##########
@@ -99,12 +114,26 @@ public String toString() {
public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
lastSequenceNumber = in.readLong();
versionTag = (VersionTag) DataSerializer.readObject(in);
+ if
(StaticSerialization.getVersionForDataStream(in).isNotOlderThan(KnownVersion.GEODE_1_15_0))
{
Review comment:
This is ok but sometimes it's easier to read if you import static
utility methods and constants:
```
import static
org.apache.geode.internal.serialization.KnownVersion.GEODE_1_15_0;
import static
org.apache.geode.internal.serialization.StaticSerialization.getVersionForDataStream;
```
```
if (getVersionForDataStream(in).isNotOlderThan(GEODE_1_15_0)) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
