joerghoh commented on code in PR #3071:
URL: https://github.com/apache/jackrabbit-oak/pull/3071#discussion_r3751567900
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheBuilder.java:
##########
@@ -296,6 +362,60 @@ private Caffeine<K, V> configureCaffeineBuilder() {
return (Caffeine<K, V>) caffeineBuilder;
}
+ /**
+ * The executor Caffeine runs cache maintenance on.
+ * <p>
+ * Oak owns this pool rather than letting Caffeine fall back to
+ * {@link java.util.concurrent.ForkJoinPool#commonPool()}, for three
reasons:
+ * <ul>
+ * <li><em>Liveness.</em> The common pool is configurable to zero workers
+ * ({@code
-Djava.util.concurrent.ForkJoinPool.common.parallelism=0}), in which case
+ * {@code execute(Runnable)} tasks are queued and never run -
eviction would stop and
+ * removal listeners would never fire, silently. The bounded queue
plus
+ * {@link ThreadPoolExecutor.CallerRunsPolicy} here guarantees
maintenance always runs
+ * eventually, degrading to the pre-OAK-12290 inline behaviour
rather than stalling.</li>
+ * <li><em>Isolation.</em> The common pool is shared with every {@code
parallelStream()} in
+ * the JVM, including the hosting application's. A saturated common
pool would delay
+ * segment-cache weight accounting and, once Caffeine's write buffer
fills, push
+ * maintenance back onto request threads - reintroducing the very
lock contention
+ * OAK-12290 is about.</li>
+ * <li><em>Diagnosability.</em> Named daemon threads make cache
maintenance identifiable in a
+ * thread dump, which is how OAK-12290 and SKYOPS-149400 were
diagnosed in the first
+ * place. This also matches Oak's existing convention of never using
the common pool
+ * (see {@code ForkJoinUtils#submitInCustomPool}).</li>
Review Comment:
This is an explanation I would expect either in Jira or the PR description,
but not in the code. For that just remove it.
```suggestion
```
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheBuilder.java:
##########
@@ -44,6 +49,52 @@
*/
public final class CacheBuilder<K, V> {
+ /**
+ * Feature toggle name for {@link
#FT_OAK_12290_ASYNC_CACHE_MAINTENANCE_ENABLED}.
+ */
+ public static final String FT_OAK_12290 = "FT_OAK-12290";
+
+ /**
+ * Whether Caffeine runs cache maintenance (eviction, removal
notification, buffer drains)
+ * on Oak's maintenance executor instead of the calling thread. Defaults
to {@code true} as a
+ * <strong>bug-fix</strong> toggle: inline maintenance made request,
indexer and writer
+ * threads hold Caffeine's eviction lock for the duration of the
maintenance work, which
+ * caused lock contention (OAK-12290) and, when a lock holder died, a
wedged JVM
+ * (SKYOPS-149400).
+ * <p>
+ * The toggle is registered on the OSGi Whiteboard under {@link
#FT_OAK_12290}. Its value is
+ * read when a cache is built, so flipping it only affects caches built
afterwards - Oak's
+ * long-lived caches are built during startup and keep the setting they
were built with.
+ * <p>
+ * Caches configured with {@link #refreshAfterWrite(Duration)} ignore the
toggle and always use
+ * the maintenance executor, because Caffeine shares one executor between
maintenance and
+ * refresh and a synchronous refresh would run the loader (potentially a
remote call) on the
+ * calling thread.
+ */
Review Comment:
The comment is way too verbose
```suggestion
* Whether Caffeine runs cache maintenance (eviction, removal
notification, buffer drains)
* on Oak's maintenance executor instead of the calling thread. Defaults
to {@code true}.
*/
```
(mind, that I also removed the reference to the SKYOPS ticket!)
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheBuilder.java:
##########
@@ -277,6 +337,12 @@ private Caffeine<K, V> configureCaffeineBuilder() {
}
if (evictionListener != null) {
EvictionListener<? super K, ? super V> listener = evictionListener;
+ // Deliberately removalListener and not evictionListener: Caffeine
invokes the latter
+ // inside the map's atomic removal, holding the bin lock for the
key. Oak's listeners do
+ // real work there - NodeCache.evicted() enqueues a
persistent-cache write under its own
+ // monitor - so that would reintroduce exactly the kind of lock
coupling OAK-12290 is
+ // about. removalListener runs on the maintenance executor
instead; listeners must
+ // therefore tolerate lagging behind the write that caused the
removal.
Review Comment:
I don't understand this comment at all ... do you want to explain why you
use ``removalListener`` instead of ``evictionListeners``? In that case I would
appreciate a simpler and more high-level explanation (and more brief would be
good as well); the current form just raises much more question marks with me.
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheBuilder.java:
##########
@@ -296,6 +362,60 @@ private Caffeine<K, V> configureCaffeineBuilder() {
return (Caffeine<K, V>) caffeineBuilder;
}
+ /**
+ * The executor Caffeine runs cache maintenance on.
+ * <p>
+ * Oak owns this pool rather than letting Caffeine fall back to
+ * {@link java.util.concurrent.ForkJoinPool#commonPool()}, for three
reasons:
+ * <ul>
+ * <li><em>Liveness.</em> The common pool is configurable to zero workers
+ * ({@code
-Djava.util.concurrent.ForkJoinPool.common.parallelism=0}), in which case
+ * {@code execute(Runnable)} tasks are queued and never run -
eviction would stop and
+ * removal listeners would never fire, silently. The bounded queue
plus
+ * {@link ThreadPoolExecutor.CallerRunsPolicy} here guarantees
maintenance always runs
+ * eventually, degrading to the pre-OAK-12290 inline behaviour
rather than stalling.</li>
+ * <li><em>Isolation.</em> The common pool is shared with every {@code
parallelStream()} in
+ * the JVM, including the hosting application's. A saturated common
pool would delay
+ * segment-cache weight accounting and, once Caffeine's write buffer
fills, push
+ * maintenance back onto request threads - reintroducing the very
lock contention
+ * OAK-12290 is about.</li>
+ * <li><em>Diagnosability.</em> Named daemon threads make cache
maintenance identifiable in a
+ * thread dump, which is how OAK-12290 and SKYOPS-149400 were
diagnosed in the first
+ * place. This also matches Oak's existing convention of never using
the common pool
+ * (see {@code ForkJoinUtils#submitInCustomPool}).</li>
+ * </ul>
+ */
+ private static Executor maintenanceExecutor() {
+ return MaintenanceExecutorHolder.EXECUTOR;
+ }
+
+ /**
+ * Lazy holder so the pool is only created once a cache is actually built.
+ */
+ private static final class MaintenanceExecutorHolder {
+
+ private static final Executor EXECUTOR = newMaintenanceExecutor();
+
+ private static Executor newMaintenanceExecutor() {
+ AtomicInteger threadCounter = new AtomicInteger();
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(
+ MAINTENANCE_THREADS, MAINTENANCE_THREADS,
+ 60, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(MAINTENANCE_QUEUE_CAPACITY),
+ runnable -> {
+ Thread thread = new Thread(runnable,
Review Comment:
You might want to add to call ``thread.setUncaughtExceptionHandler()`` as
well to just log the exception.
--
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]
