jsedding commented on code in PR #3071:
URL: https://github.com/apache/jackrabbit-oak/pull/3071#discussion_r3766290434
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentId.java:
##########
@@ -221,6 +233,19 @@ void unloaded() {
this.segment = null;
}
+ /**
+ * Like {@link #unloaded()}, but only clears the memoised segment if it is
still
+ * {@code expected}. Use this for asynchronous removal notifications,
where {@code expected}
+ * may already have been superseded by a concurrent {@link
#loaded(Segment)}.
+ *
+ * @param expected the segment that was removed; the field is left
untouched if it no longer
+ * holds this value
+ * @return {@code true} if {@code expected} was still memoised and has now
been cleared
+ */
+ boolean unloadIfCurrent(@NotNull Segment expected) {
+ return SEGMENT.compareAndSet(this, expected, null);
+ }
Review Comment:
I would suggest to change the name to just `unload` or `compareAndUnload`.
For me, the current name was more confusing than helpful. Now I understand
your intention, but it didn't immediately make sense.
```suggestion
boolean compareAndUnload(@NotNull Segment expected) {
return SEGMENT.compareAndSet(this, expected, null);
}
```
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/EvictionListener.java:
##########
@@ -22,9 +22,13 @@
/**
* Callback invoked when an entry is removed from the cache.
*
- * <p>Register via {@link CacheBuilder#evictionListener(EvictionListener)}.
- * The callback is invoked synchronously during cache operations that trigger
- * removal (eviction, invalidation, replacement).</p>
+ * <p>Register via {@link CacheBuilder#evictionListener(EvictionListener)}.</p>
+ *
+ * <p>The callback runs on the cache's maintenance executor, not on the
triggering thread, so it
+ * may lag behind and observe a key that was already re-inserted. Use {@link
Cache#cleanUp()} to
+ * drain pending callbacks before relying on external accounting they
maintain. Exception: with
+ * {@link CacheBuilder#FT_OAK_12290_ASYNC_CACHE_MAINTENANCE_ENABLED} disabled,
the callback runs
+ * synchronously on the triggering thread instead, as it always did before
that toggle existed.</p>
Review Comment:
Again, I would leave implementation details out of API documentation.
```suggestion
* <p>The callback <i>may</> run asynchronously. Implementations should
expect that they may lag
* behind and observe a key that was already re-inserted. Use {@link
Cache#cleanUp()} to
* drain pending callbacks before relying on external accounting they
maintain.</p>
```
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentNodeStoreRegistrar.java:
##########
@@ -282,6 +283,12 @@ private SegmentNodeStore register() throws IOException {
new FeatureToggle(SegmentCache.FT_OAK_12214,
SegmentCache.FT_OAK_12214_PROPAGATE_L1_HITS_TO_L2_ENABLED),
Collections.emptyMap()));
+ // OAK-12290: bug-fix toggle (default on) so Caffeine maintenance
never runs inline
+ // on request, indexer or writer threads while holding the eviction
lock
Review Comment:
remove AI comment
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/Cache.java:
##########
@@ -144,9 +144,10 @@ public interface Cache<K, V> {
/**
* Performs any pending maintenance operations needed by the cache.
*
- * <p><em>Note: no Oak module currently calls this method; the CacheLIRS
- * implementation is a no-op. It may be removed from the interface in a
- * future release if it remains unused.</em></p>
+ * <p>Applies pending evictions to the cache's internal state before
returning, but does not
+ * wait for their {@link EvictionListener} callbacks, which still run
asynchronously (unless
+ * async cache maintenance is disabled, in which case those callbacks
already ran inline
+ * before this method was even called). The CacheLIRS implementation is a
no-op.</p>
Review Comment:
This should describe the API contract rather than the various
implementations. E.g.
```suggestion
* <p>Applies pending evictions to the cache's internal state before
returning, but
* {@link EvictionListener} callbacks may run asynchronously. Thus there
is no
* guarantee that {@link EvictionListener} callbacks have been called or
have
* completed when this call returns.
```
--
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]
