jsedding commented on code in PR #2864:
URL: https://github.com/apache/jackrabbit-oak/pull/2864#discussion_r3092410356
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/AbstractCacheStats.java:
##########
@@ -38,8 +38,8 @@ public abstract class AbstractCacheStats extends
AnnotatedStandardMBean implemen
@NotNull
private final String name;
- private CacheStats lastSnapshot =
- new CacheStats(0, 0, 0, 0, 0, 0);
+ private CacheStatsSnapshot lastSnapshot =
+ new CacheStatsSnapshot(0, 0, 0, 0, 0, 0);
Review Comment:
`lastSnapshot` should be `volatile`. It isn't properly `synchronized`
anyway, and synchronization is not nessesary.
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/AbstractCacheStats.java:
##########
Review Comment:
This probably should move to the `org.apache.jackrabbit.oak.cache.api`, I
think. The class is used by other bundles, right? So it needs to be exported,
and probably annotated `@ProviderType`.
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/AbstractCacheStats.java:
##########
@@ -51,19 +51,17 @@ protected AbstractCacheStats(@NotNull String name) {
}
/**
- * Call back invoked to retrieve the most recent {@code CacheStats}
instance of the
+ * Call back invoked to retrieve the most recent {@link
CacheStatsSnapshot} of the
* underlying cache.
*/
- protected abstract CacheStats getCurrentStats();
+ protected abstract CacheStatsSnapshot getCurrentStats();
- private CacheStats stats() {
+ private CacheStatsSnapshot stats() {
return getCurrentStats().minus(lastSnapshot);
}
Review Comment:
Together with the proposal to make `lastSnapshot` volatile, we should make
sure to first get the value of `lastSnapshot` before calling
`getCurrentStats(). Otherwise it is possible that a `resetStats()` call happens
after `getCurrentStats()` and reading `lastSnapshot`, leading to negative
values.
```suggestion
private CacheStatsSnapshot stats() {
// get baseline before calling getCurrentStats() to guard against
race with resetStats()
CacheStatsSnapshot baseline = lastSnapshot;
return getCurrentStats().minus(baseline);
}
```
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/AbstractCacheStats.java:
##########
@@ -51,19 +51,17 @@ protected AbstractCacheStats(@NotNull String name) {
}
/**
- * Call back invoked to retrieve the most recent {@code CacheStats}
instance of the
+ * Call back invoked to retrieve the most recent {@link
CacheStatsSnapshot} of the
* underlying cache.
*/
- protected abstract CacheStats getCurrentStats();
+ protected abstract CacheStatsSnapshot getCurrentStats();
- private CacheStats stats() {
+ private CacheStatsSnapshot stats() {
return getCurrentStats().minus(lastSnapshot);
}
@Override
public synchronized void resetStats() {
Review Comment:
If we switch `lastSnapshot` to volatile, this no longer needs to be
synchronized.
```suggestion
public void resetStats() {
```
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/WriterCacheManager.java:
##########
@@ -351,17 +351,20 @@ public CacheStatsMBean getTemplateCacheStats() {
}
@NotNull
- private static <T> Supplier<CacheStats> accumulateRecordCacheStats(
+ private static <T> Supplier<CacheStatsSnapshot>
accumulateRecordCacheStats(
final Iterable<RecordCache<T>> caches) {
- return new Supplier<CacheStats>() {
- @Override
- public CacheStats get() {
- CacheStats stats = new CacheStats(0, 0, 0, 0, 0, 0);
- for (RecordCache<?> cache : caches) {
- stats = stats.plus(cache.getStats());
- }
- return stats;
+ return () -> {
+ long hits = 0, misses = 0, loads = 0, failures = 0, loadTime =
0, evictions = 0;
+ for (RecordCache<?> cache : caches) {
+ CacheStatsSnapshot s = cache.getStats();
+ hits += s.hitCount();
+ misses += s.missCount();
+ loads += s.loadSuccessCount();
+ failures += s.loadFailureCount();
+ loadTime += s.totalLoadTime();
+ evictions += s.evictionCount();
Review Comment:
We could consider implementing
`CacheStatsSnapshot#plus(CacheStatsSnapshot)`. It already implements
`#minus(CacheStatsSnapshot)`.
##########
oak-core-spi/src/test/java/org/apache/jackrabbit/oak/cache/api/CacheStatsTest.java:
##########
@@ -20,7 +20,7 @@
import org.junit.Test;
/** Tests for {@link CacheStatsSnapshot}. */
-public class CacheStatsSnapshotTest {
+public class CacheStatsTest {
Review Comment:
The name should be `CacheStatsSnapshotTest`.
--
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]
