You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by dl...@apache.org on 2022/05/11 16:09:31 UTC
[accumulo] branch main updated: Execute Caffeine tasks in current Thread for tests (#2691)
This is an automated email from the ASF dual-hosted git repository.
dlmarion pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push:
new 30a74e2400 Execute Caffeine tasks in current Thread for tests (#2691)
30a74e2400 is described below
commit 30a74e24008e6015d56443cebb348d257ba6420d
Author: Dave Marion <dl...@apache.org>
AuthorDate: Wed May 11 12:09:26 2022 -0400
Execute Caffeine tasks in current Thread for tests (#2691)
* Execute Caffeine tasks in current Thread for tests
As described at https://github.com/ben-manes/caffeine/wiki/Testing, Caffeine
delegates maintenance tasks (eviction, refreshes, etc.) to background threads
to take advantage of batching. This change implements the suggestion at
https://github.com/ben-manes/caffeine/issues/455#issuecomment-689398669 to
run all of the maintenance tasks in the current Thread for testing. Using
Cache.cleanup as suggested in the wiki page did not appear to work, but this
approach allows us to remove all of the calls to Thread.sleep in the tests.
---
.../conf/store/impl/PropCacheCaffeineImpl.java | 23 +++++------
.../server/conf/store/impl/ZooPropStore.java | 9 +----
.../conf/store/impl/PropCacheCaffeineImplTest.java | 6 +--
.../server/conf/store/impl/ZooPropLoaderTest.java | 47 +++++++---------------
.../accumulo/test/conf/store/ZooBasedConfigIT.java | 2 -
5 files changed, 28 insertions(+), 59 deletions(-)
diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImpl.java b/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImpl.java
index d1664b8fcb..6433a51a4c 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImpl.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImpl.java
@@ -35,7 +35,6 @@ import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.github.benmanes.caffeine.cache.RemovalCause;
import com.github.benmanes.caffeine.cache.Ticker;
-import com.google.common.annotations.VisibleForTesting;
public class PropCacheCaffeineImpl implements PropCache {
@@ -51,11 +50,15 @@ public class PropCacheCaffeineImpl implements PropCache {
private final LoadingCache<PropCacheKey<?>,VersionedProperties> cache;
private PropCacheCaffeineImpl(final CacheLoader<PropCacheKey<?>,VersionedProperties> cacheLoader,
- final PropStoreMetrics metrics, final Ticker ticker) {
+ final PropStoreMetrics metrics, final Ticker ticker, boolean runTasksInline) {
this.metrics = metrics;
var builder = Caffeine.newBuilder().refreshAfterWrite(REFRESH_MIN, BASE_TIME_UNITS)
- .expireAfterAccess(EXPIRE_MIN, BASE_TIME_UNITS).evictionListener(this::evictionNotifier)
- .executor(executor);
+ .expireAfterAccess(EXPIRE_MIN, BASE_TIME_UNITS).evictionListener(this::evictionNotifier);
+ if (runTasksInline) {
+ builder = builder.executor(Runnable::run);
+ } else {
+ builder = builder.executor(executor);
+ }
if (ticker != null) {
builder = builder.ticker(ticker);
}
@@ -95,12 +98,6 @@ public class PropCacheCaffeineImpl implements PropCache {
cache.invalidateAll();
}
- /** for testing - force Caffeine housekeeping to run. */
- @VisibleForTesting
- void cleanUp() {
- cache.cleanUp();
- }
-
/**
* Retrieve the version properties if present in the cache, otherwise return null. This prevents
* caching the properties and should be used when properties will be updated and then committed to
@@ -122,6 +119,7 @@ public class PropCacheCaffeineImpl implements PropCache {
private final PropStoreMetrics metrics;
private final ZooPropLoader zooPropLoader;
private Ticker ticker = null;
+ private boolean runTasksInline = false;
public Builder(final ZooPropLoader zooPropLoader, final PropStoreMetrics metrics) {
Objects.requireNonNull(zooPropLoader, "A PropStoreChangeMonitor must be provided");
@@ -130,11 +128,12 @@ public class PropCacheCaffeineImpl implements PropCache {
}
public PropCacheCaffeineImpl build() {
- return new PropCacheCaffeineImpl(zooPropLoader, metrics, ticker);
+ return new PropCacheCaffeineImpl(zooPropLoader, metrics, ticker, runTasksInline);
}
- public Builder withTicker(final Ticker ticker) {
+ public Builder forTests(final Ticker ticker) {
this.ticker = ticker;
+ this.runTasksInline = true;
return this;
}
}
diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java b/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java
index 7c522e0e2f..f8449f0497 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java
@@ -46,7 +46,6 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.github.benmanes.caffeine.cache.Ticker;
-import com.google.common.annotations.VisibleForTesting;
public class ZooPropStore implements PropStore, PropChangeListener {
@@ -103,7 +102,7 @@ public class ZooPropStore implements PropStore, PropChangeListener {
this.cache = new PropCacheCaffeineImpl.Builder(propLoader, cacheMetrics).build();
} else {
this.cache =
- new PropCacheCaffeineImpl.Builder(propLoader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(propLoader, cacheMetrics).forTests(ticker).build();
}
MetricsUtil.initializeProducers(cacheMetrics);
@@ -355,12 +354,6 @@ public class ZooPropStore implements PropStore, PropChangeListener {
cache.removeAll();
}
- /** for testing - force Caffeine housekeeping to run. */
- @VisibleForTesting
- public void cleanUp() {
- ((PropCacheCaffeineImpl) cache).cleanUp();
- }
-
/**
* Read and decode property node from ZooKeeper.
*
diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImplTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImplTest.java
index d6a766e7d3..6b6b2858ae 100644
--- a/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImplTest.java
+++ b/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/PropCacheCaffeineImplTest.java
@@ -87,8 +87,7 @@ public class PropCacheCaffeineImplTest {
expect(context.getInstanceID()).andReturn(instanceId).anyTimes();
- cache =
- new PropCacheCaffeineImpl.Builder(zooPropLoader, cacheMetrics).withTicker(ticker).build();
+ cache = new PropCacheCaffeineImpl.Builder(zooPropLoader, cacheMetrics).forTests(ticker).build();
}
@@ -175,11 +174,9 @@ public class PropCacheCaffeineImplTest {
assertNotNull(cache.get(tablePropKey)); // will call load and place into cache
ticker.advance(30, TimeUnit.MINUTES);
- cache.cleanUp();
assertNotNull(cache.get(tablePropKey)); // will async check stat and then reload
}
- @SuppressWarnings({"rawtypes"})
@Test
public void expireTest() {
expect(zooPropLoader.load(eq(tablePropKey))).andReturn(vProps).times(2);
@@ -188,7 +185,6 @@ public class PropCacheCaffeineImplTest {
assertNotNull(cache.get(tablePropKey)); // will call load
ticker.advance(90, TimeUnit.MINUTES);
- cache.cleanUp();
assertNotNull(cache.get(tablePropKey)); // expired - will call load.
}
diff --git a/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/ZooPropLoaderTest.java b/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/ZooPropLoaderTest.java
index 84a809bd1c..ba376dc22d 100644
--- a/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/ZooPropLoaderTest.java
+++ b/server/base/src/test/java/org/apache/accumulo/server/conf/store/impl/ZooPropLoaderTest.java
@@ -148,7 +148,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load into cache
assertNotNull(cache.get(propCacheKey));
@@ -186,7 +186,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
assertNull(cache.get(propCacheKey));
@@ -215,13 +215,12 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load cache
assertNotNull(cache.get(propCacheKey));
ticker.advance(70, TimeUnit.MINUTES);
- cache.cleanUp();
assertNotNull(cache.get(propCacheKey));
@@ -267,26 +266,21 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// prime cache
assertNotNull(cache.get(propCacheKey));
ticker.advance(5, TimeUnit.MINUTES);
- cache.cleanUp();
// read cached value
assertNotNull(cache.get(propCacheKey));
// advance so refresh called.
ticker.advance(20, TimeUnit.MINUTES);
- cache.cleanUp();
assertNotNull(cache.get(propCacheKey));
- // yield so async thread completes.
- Thread.sleep(250);
-
assertNull(cache.get(propCacheKey));
}
@@ -296,7 +290,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
assertNull(cache.getWithoutCaching(propCacheKey));
@@ -320,14 +314,13 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load into cache
assertNotNull(cache.get(sysPropKey));
assertNotNull(cache.get(tablePropKey));
cache.remove(tablePropKey);
- cache.cleanUp();
// verify retrieved from cache without loading.
assertNotNull(cache.getWithoutCaching(sysPropKey));
@@ -352,14 +345,13 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load into cache
assertNotNull(cache.get(sysPropKey));
assertNotNull(cache.get(tablePropKey));
cache.removeAll();
- cache.cleanUp();
// verify retrieved from cache without loading.
assertNull(cache.getWithoutCaching(sysPropKey));
@@ -371,7 +363,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load into cache
assertNull(cache.getWithoutCaching(propCacheKey));
@@ -406,7 +398,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load cache
log.debug("received: {}", cache.get(propCacheKey));
@@ -419,14 +411,10 @@ public class ZooPropLoaderTest {
assertNotNull(cache.get(propCacheKey));
- Thread.sleep(100);
-
ticker.advance(REFRESH_MIN + 1, TimeUnit.MINUTES);
assertNotNull(cache.get(propCacheKey));
- Thread.sleep(100);
-
ticker.advance(1, TimeUnit.MINUTES);
assertNotNull(cache.get(propCacheKey));
@@ -490,10 +478,12 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// prime cache
- assertNotNull(cache.get(propCacheKey));
+ var origProps = cache.get(propCacheKey);
+ assertNotNull(origProps);
+ assertEquals("7G", origProps.asMap().get(Property.TABLE_SPLIT_THRESHOLD.getKey()));
ticker.advance(REFRESH_MIN + 1, TimeUnit.MINUTES);
// first call after refresh return original and schedules update
@@ -501,15 +491,11 @@ public class ZooPropLoaderTest {
assertNotNull(originalProps);
assertNotNull(originalProps.asMap().get(Property.TABLE_SPLIT_THRESHOLD.getKey()));
- // allow refresh thread to run
- Thread.sleep(50);
-
// refresh should have loaded updated value;
var updatedProps = cache.get(propCacheKey);
log.debug("Updated props: {}", updatedProps == null ? "null" : updatedProps.print(true));
assertNotNull(updatedProps);
- Thread.sleep(250);
assertEquals("12G", updatedProps.asMap().get(Property.TABLE_SPLIT_THRESHOLD.getKey()));
}
@@ -558,19 +544,17 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics, mockProps);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// prime cache
cache.get(propCacheKey);
ticker.advance(30, TimeUnit.MINUTES);
- cache.cleanUp();
VersionedProperties vPropsRead = cache.get(propCacheKey);
assertNotNull(vPropsRead);
- Thread.sleep(250);
cache.get(propCacheKey);
verify(mockProps);
@@ -619,7 +603,7 @@ public class ZooPropLoaderTest {
replay(context, zrw, propStoreWatcher, cacheMetrics);
PropCacheCaffeineImpl cache =
- new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).withTicker(ticker).build();
+ new PropCacheCaffeineImpl.Builder(loader, cacheMetrics).forTests(ticker).build();
// load cache
log.debug("received: {}", cache.get(propCacheKey));
@@ -628,7 +612,6 @@ public class ZooPropLoaderTest {
assertNotNull(cache.get(propCacheKey)); // returns current and queues async refresh
- Thread.sleep(150);
assertNull(cache.get(propCacheKey)); // on exception, the loader should return null
}
diff --git a/test/src/main/java/org/apache/accumulo/test/conf/store/ZooBasedConfigIT.java b/test/src/main/java/org/apache/accumulo/test/conf/store/ZooBasedConfigIT.java
index 34a32333ec..a6ab8fe8a4 100644
--- a/test/src/main/java/org/apache/accumulo/test/conf/store/ZooBasedConfigIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/conf/store/ZooBasedConfigIT.java
@@ -281,8 +281,6 @@ public class ZooBasedConfigIT {
// advance well past unload period.
ticker.advance(2, TimeUnit.HOURS);
- // force clean-up and cache activity to get async unload to occur.
- ((ZooPropStore) propStore).cleanUp();
var tableBPropKey = TablePropKey.of(INSTANCE_ID, tidB);
propStore.create(tableBPropKey, Map.of());
Thread.sleep(150);