pivotal-jbarrett commented on code in PR #7381:
URL: https://github.com/apache/geode/pull/7381#discussion_r857803710
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java:
##########
@@ -91,30 +99,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
Review Comment:
Please rename the test to the current naming convention,
`UpdatePropogationDistributedTest`.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java:
##########
@@ -91,30 +99,63 @@ public final void postSetUp() throws Exception {
// Server2 VM
server2 = host.getVM(1);
+ // Server3 VM
Review Comment:
These comments don't provide any more detail than the variable name, so they
can just go away.
##########
geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java:
##########
@@ -458,8 +472,14 @@ private ConnectExceptions getConnections(Membership mgr,
DistributionMessage msg
if (ackTimeout > 0) {
startTime = System.currentTimeMillis();
}
- Connection con = conduit.getConnection(destination, preserveOrder,
retry, startTime,
- ackTimeout, ackSDTimeout);
+ Connection con;
Review Comment:
Can this be `final`?
Perhaps this if block should be extracted into its own method for clarity.
I wouldn't mind a rename of `con` to `connection` while we are here.
##########
geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java:
##########
@@ -909,6 +906,104 @@ public Connection getConnection(InternalDistributedMember
memberAddress,
}
}
+
+ /**
+ * Return a connection to the given member. This method performs quick scan
for connection.
+ * Only one attempt to create a connection to the given member .
+ *
+ * @param memberAddress the IDS associated with the remoteId
+ * @param preserveOrder whether this is an ordered or unordered connection
+ * @param startTime the time this operation started
+ * @param ackTimeout the ack-wait-threshold * 1000 for the operation to be
transmitted (or zero)
+ * @param ackSATimeout the ack-severe-alert-threshold * 1000 for the
operation to be transmitted
+ * (or zero)
+ *
+ * @return the connection
+ */
+ public Connection getFirstScanForConnection(InternalDistributedMember
memberAddress,
+ final boolean preserveOrder, long startTime, long ackTimeout,
+ long ackSATimeout) throws IOException,
DistributedSystemDisconnectedException {
+ if (stopped) {
+ throw new DistributedSystemDisconnectedException("The conduit is
stopped");
+ }
+
+ InternalDistributedMember memberInTrouble = null;
+ Connection conn = null;
Review Comment:
Please don't use abbreviated variable names.
##########
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java:
##########
@@ -400,6 +401,10 @@ private Connection
getSharedConnection(InternalDistributedMember id, boolean sch
throw new IOException("Cannot form connection to alert listener " +
id);
}
+ if (onlyOneTry) {
Review Comment:
This doesn't feel like an appropriately named variable here. It feels more
like a "do not wait for connection".
##########
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java:
##########
@@ -1021,7 +1021,9 @@ static Connection createSender(final
Membership<InternalDistributedMember> mgr,
// do not change the text of this exception - it is looked for in
exception handlers
throw new IOException("Cannot form connection to alert listener "
+ remoteAddr);
}
-
+ if (onlyOneTry) {
Review Comment:
is `onlyOneTry` really "fail on first try" or "do not retry"?
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java:
##########
@@ -91,30 +99,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:
This should be an awaited assertion rather than arbitrary time. Sleeps like
this eventually result in flaky behavior under different system loads.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java:
##########
@@ -305,6 +358,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);
Review Comment:
Don't use raw types. Please add type params.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/UpdatePropagationDUnitTest.java:
##########
@@ -305,6 +358,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) {
Review Comment:
```java
} catch (Exception ignored) {
}
```
--
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]
