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/04/25 16:27:29 UTC
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7381: GEODE-9484: Improve sending message to multy destinations
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org