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/28 00:12:20 UTC
[GitHub] [geode] DonalEvans commented on a diff in pull request #7608: Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing
DonalEvans commented on code in PR #7608:
URL: https://github.com/apache/geode/pull/7608#discussion_r860287049
##########
geode-core/src/distributedTest/java/org/apache/geode/cache/client/ClientServerRegisterInterestsDUnitTest.java:
##########
@@ -191,25 +176,23 @@ private ClientCache setupGemFireClientCache() {
return clientCache;
}
- @SuppressWarnings("unchecked")
- protected <K, V> V put(final String regionName, final K key, final V value) {
- return (V) gemfireServerVm.invoke(new SerializableCallable() {
- @Override
- public Object call() throws Exception {
- Cache cache = CacheFactory.getAnyInstance();
- cache.getRegion(regionName).put(key, value);
- return cache.getRegion(regionName).get(key);
- }
+ protected <V> V put() {
+ return (V) gemfireServerVm.invoke(() -> {
+ Cache cache = CacheFactory.getAnyInstance();
+ cache.getRegion("/Example").put("2", "TWO");
+ return cache.getRegion("/Example").get("2");
});
}
- protected void waitOnEvent(final long waitTimeMilliseconds) {
- final long timeout = (System.currentTimeMillis() + waitTimeMilliseconds);
+ protected void waitOnEvent() {
+ final long timeout = (System.currentTimeMillis()
+ + ClientServerRegisterInterestsDUnitTest.WAIT_TIME_MILLISECONDS);
while (entryEvents.empty() && (System.currentTimeMillis() < timeout)) {
synchronized (this) {
try {
- TimeUnit.MILLISECONDS.timedWait(this, Math.min(500, waitTimeMilliseconds));
+ TimeUnit.MILLISECONDS.timedWait(this, Math.min(500,
+ ClientServerRegisterInterestsDUnitTest.WAIT_TIME_MILLISECONDS));
} catch (InterruptedException ignore) {
}
Review Comment:
This whole method should probably just be replaced with:
```
await().untilAsserted(() -> assertThat(entryEvents).isNotEmpty());
```
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientNoServerAvailabileDistributedTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.tier.sockets;
+
+import static org.apache.geode.distributed.ConfigurationProperties.DURABLE_CLIENT_ID;
+import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
+import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.Properties;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.NoSubscriptionServersAvailableException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionFactory;
+import org.apache.geode.cache.Scope;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
+import org.apache.geode.test.dunit.internal.JUnit4DistributedTestCase;
+import org.apache.geode.test.junit.categories.ClientSubscriptionTest;
+
+@Category({ClientSubscriptionTest.class})
+public class DurableClientNoServerAvailabileDistributedTest extends JUnit4CacheTestCase {
Review Comment:
Typo here, should be "NoServerAvailable"
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java:
##########
@@ -272,9 +273,12 @@ public static void checkStatisticsWithExpectedValues(int reconnectionCount, int
logger.info("Stats:" + "\nDurableReconnectionCount:" + stats.get_durableReconnectionCount()
+ "\nQueueDroppedCount" + stats.get_queueDroppedCount()
+ "\nEventsEnqueuedWhileClientAwayCount" + stats.get_eventEnqueuedWhileClientAwayCount());
- assertThat(stats.get_durableReconnectionCount()).isEqualTo(reconnectionCount);
- assertThat(stats.get_queueDroppedCount()).isEqualTo(queueDropCount);
- assertThat(stats.get_eventEnqueuedWhileClientAwayCount()).isEqualTo(enqueueCount);
+ await().untilAsserted(
+ () -> assertThat(stats.get_durableReconnectionCount()).isEqualTo(reconnectionCount));
+ await()
+ .untilAsserted(() -> assertThat(stats.get_queueDroppedCount()).isEqualTo(queueDropCount));
+ await().untilAsserted(
+ () -> assertThat(stats.get_eventEnqueuedWhileClientAwayCount()).isEqualTo(enqueueCount));
} catch (Exception e) {
fail("Exception thrown while executing checkStatisticsWithExpectedValues()", e);
Review Comment:
`AssertionError` which is thrown by the assertions in this try block is an `Error` rather than an `Exception` so it won't be caught here and any assertions that fail will bubble up. I personally dislike the use of try/catch coupled with `fail()` since it's not really adding anything here that wouldn't be clear from the stack trace of the exception, and it introduces inconsistent behaviour depending on the type of error/exception we might encounter.
##########
geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java:
##########
@@ -508,6 +494,11 @@ public boolean waitWhileNotEnoughEvents(long sleepMs, int eventCount, int eventT
return waitWhileNotEnoughEvents(sleepMs, eventCount, getEvents(eventType));
}
+
+ /**
+ * This method is not implemented to test event count matches the eventsToCheck.size() which is
+ * confusing. It will wait and return if it got something in the eventsToCheck or not.
+ */
Review Comment:
This should either use correct javadoc semantics (add a `@param` tag for each argument and an `@return` tag for the returned value) or it should be a block comment rather than a javadoc comment.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java:
##########
@@ -69,62 +75,57 @@ public class DurableClientStatsDUnitTest extends JUnit4DistributedTestCase {
private int PORT1;
- private final String K1 = "Key1";
-
@Override
- public final void postSetUp() throws Exception {
- Host host = Host.getHost(0);
- server1VM = host.getVM(0);
- durableClientVM = host.getVM(1);
+ public final void postSetUp() {
+ server1VM = VM.getVM(0);
+ durableClientVM = VM.getVM(1);
regionName = DurableClientStatsDUnitTest.class.getName() + "_region";
- CacheServerTestUtil.disableShufflingOfEndpoints();
+ disableShufflingOfEndpoints();
}
@Override
- public final void preTearDown() throws Exception {
+ public final void preTearDown() {
// Stop server 1
server1VM.invoke(() -> CacheServerTestUtil.closeCache());
- CacheServerTestUtil.resetDisableShufflingOfEndpointsFlag();
+ resetDisableShufflingOfEndpointsFlag();
}
@Test
public void testNonDurableClientStatistics() {
// Step 1: Starting the servers
PORT1 = server1VM
- .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
+ .invoke(() -> createCacheServer(regionName, true));
server1VM.invoke(DurableClientStatsDUnitTest::checkStatistics);
// Step 2: Bring Up the Client
// Start a durable client that is not kept alive on the server when it
// stops normally
- final int durableClientTimeout = 600; // keep the client alive for 600
- // seconds
- startAndCloseNonDurableClientCache(durableClientTimeout);
- startAndCloseNonDurableClientCache(1); //////// -> Reconnection1
+ startAndCloseNonDurableClientCache();
+ startAndCloseNonDurableClientCache(); //////// -> Reconnection1
Wait.pause(1400); //////// -> Queue Dropped1
Review Comment:
These waits should ideally be replaced with `await()` calls on some reasonable condition. For this one and the one below it the following seems right:
```
await().untilAsserted(() -> assertThat(durableClientVM.invoke(() -> getClientCache().isClosed())).isTrue());
```
##########
geode-core/src/distributedTest/java/org/apache/geode/cache/client/ClientServerRegisterInterestsDUnitTest.java:
##########
@@ -191,25 +176,23 @@ private ClientCache setupGemFireClientCache() {
return clientCache;
}
- @SuppressWarnings("unchecked")
- protected <K, V> V put(final String regionName, final K key, final V value) {
- return (V) gemfireServerVm.invoke(new SerializableCallable() {
- @Override
- public Object call() throws Exception {
- Cache cache = CacheFactory.getAnyInstance();
- cache.getRegion(regionName).put(key, value);
- return cache.getRegion(regionName).get(key);
- }
+ protected <V> V put() {
+ return (V) gemfireServerVm.invoke(() -> {
+ Cache cache = CacheFactory.getAnyInstance();
+ cache.getRegion("/Example").put("2", "TWO");
+ return cache.getRegion("/Example").get("2");
});
}
Review Comment:
Warnings here can be fixed by using:
```
protected String put() {
return gemfireServerVm.invoke(() -> {
Cache cache = CacheFactory.getAnyInstance();
cache.getRegion("/Example").put("2", "TWO");
return cache.<String, String>getRegion("/Example").get("2");
});
}
```
Since we explicitly create the Region as `<String, String>`, there's no need to use generics here.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java:
##########
@@ -229,55 +215,40 @@ public void startAndCloseDurableClientCache(int durableClientTimeout) {
final String durableClientId = getName() + "_client";
durableClientVM
- .invoke(() -> CacheServerTestUtil.createCacheClient(
- getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, true,
- 0),
+ .invoke(() -> createCacheClient(
+ getClientPool(NetworkUtils.getServerHostName(), PORT1),
regionName,
getDurableClientDistributedSystemProperties(durableClientId, durableClientTimeout),
- Boolean.TRUE));
+ true));
// Send clientReady message
- durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
+ durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {
@Override
public void run2() throws CacheException {
- CacheServerTestUtil.getCache().readyForEvents();
+ getClientCache().readyForEvents();
}
});
Review Comment:
This can be simplified to:
```
durableClientVM.invoke("Send clientReady", () -> getClientCache().readyForEvents());
```
There are a lot of uses of `new CacheSerializableRunnable()` in these classes that can be replaced with a lambda like above. Doing so no only simplifies the code, but removes warnings due to the `CacheSerializableRunnable` not defining a serialVersionUID.
##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientNoServerAvailabileDistributedTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.tier.sockets;
+
+import static org.apache.geode.distributed.ConfigurationProperties.DURABLE_CLIENT_ID;
+import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
+import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.Properties;
+
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.NoSubscriptionServersAvailableException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionFactory;
+import org.apache.geode.cache.Scope;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
+import org.apache.geode.test.dunit.internal.JUnit4DistributedTestCase;
+import org.apache.geode.test.junit.categories.ClientSubscriptionTest;
+
+@Category({ClientSubscriptionTest.class})
+public class DurableClientNoServerAvailabileDistributedTest extends JUnit4CacheTestCase {
+
+ @Override
+ public final void postTearDownCacheTestCase() {
+ VM.getVM(0).invoke(JUnit4DistributedTestCase::disconnectFromDS);
+ }
+
+ @Test
+ public void testNoServerAvailableOnStartup() {
+ VM vm0 = VM.getVM(0);
+ VM vm1 = VM.getVM(1);
+
+ final String hostName = VM.getHostName();
+ final int port = AvailablePortHelper.getRandomAvailableTCPPort();
+ vm0.invoke("create cache", () -> {
+ getSystem(getClientProperties());
+ PoolImpl p = (PoolImpl) PoolManager.createFactory().addServer(hostName, port)
Review Comment:
This can be the interface `Pool` rather than the concrete `PoolImpl`.
##########
geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java:
##########
@@ -192,16 +179,15 @@ public static void unsetJavaSystemProperties(Properties javaSystemProperties) {
}
}
- public static void createCacheClient(Pool poolAttr, String regionName1, String regionName2)
- throws Exception {
+ public static void createCacheClient(Pool poolAttr, String regionName1, String regionName2) {
new CacheServerTestUtil().createCache(getClientProperties());
PoolFactoryImpl pf = (PoolFactoryImpl) PoolManager.createFactory();
pf.init(poolAttr);
PoolImpl p = (PoolImpl) pf.create("CacheServerTestUtil");
- AttributesFactory factory = new AttributesFactory();
+ AttributesFactory<Object, Object> factory = new AttributesFactory<>();
factory.setScope(Scope.LOCAL);
factory.setPoolName(p.getName());
- RegionAttributes attrs = factory.create();
+ RegionAttributes<Object, Object> attrs = factory.create();
cache.createRegion(regionName1, attrs);
cache.createRegion(regionName2, attrs);
pool = p;
Review Comment:
The `addControlListener` arguments to `createCacheClientFromXmlN()` on line 197 and `createCacheClientFromXml()` on line 219 are not used and can be removed.
##########
geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java:
##########
@@ -170,20 +157,20 @@ public static void createCacheClient(Pool poolAttr, String regionName, Propertie
PoolFactoryImpl pf = (PoolFactoryImpl) PoolManager.createFactory();
pf.init(poolAttr);
PoolImpl p = (PoolImpl) pf.create("CacheServerTestUtil");
- AttributesFactory factory = new AttributesFactory();
+ AttributesFactory<Object, Object> factory = new AttributesFactory<>();
factory.setScope(Scope.LOCAL);
factory.setPoolName(p.getName());
if (addControlListener) {
factory.addCacheListener(new ControlListener());
}
- RegionAttributes attrs = factory.create();
+ RegionAttributes<Object, Object> attrs = factory.create();
cache.createRegion(regionName, attrs);
pool = p;
}
public static void unsetJavaSystemProperties(Properties javaSystemProperties) {
if (javaSystemProperties != null && javaSystemProperties.size() > 0) {
- Enumeration e = javaSystemProperties.propertyNames();
+ Enumeration<?> e = javaSystemProperties.propertyNames();
while (e.hasMoreElements()) {
String key = (String) e.nextElement();
Review Comment:
This block can be simplified to:
```
if (javaSystemProperties != null && javaSystemProperties.size() > 0) {
for (String key : javaSystemProperties.stringPropertyNames()) {
System.clearProperty(key);
}
}
```
##########
geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java:
##########
@@ -154,12 +141,12 @@ public static void createCacheClient(Pool poolAttr, String regionName, Propertie
}
public static void createCacheClient(Pool poolAttr, String regionName, Properties dsProperties,
- Boolean addControlListener, Properties javaSystemProperties) throws Exception {
+ Boolean addControlListener, Properties javaSystemProperties) {
new CacheServerTestUtil().createCache(dsProperties);
IgnoredException.addIgnoredException("java.net.ConnectException||java.net.SocketException");
if (javaSystemProperties != null && javaSystemProperties.size() > 0) {
- Enumeration e = javaSystemProperties.propertyNames();
+ Enumeration<?> e = javaSystemProperties.propertyNames();
while (e.hasMoreElements()) {
String key = (String) e.nextElement();
Review Comment:
This block can be simplified to:
```
if (javaSystemProperties != null && javaSystemProperties.size() > 0) {
for (String key : javaSystemProperties.stringPropertyNames()) {
System.setProperty(key, javaSystemProperties.getProperty(key));
}
}
```
--
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