jsedding commented on code in PR #2842:
URL: https://github.com/apache/jackrabbit-oak/pull/2842#discussion_r3071402505
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheBuilder.java:
##########
@@ -171,6 +173,19 @@ public CacheBuilder<K, V> refreshAfterWrite(@NotNull
Duration duration) {
return this;
}
+ /**
+ * Sets the nanosecond ticker used to measure time for expiry and refresh.
+ * Intended for testing with a controllable clock.
+ *
+ * @param ticker a supplier returning the current time in nanoseconds
(must not be null)
+ * @return this builder
+ */
+ @NotNull
+ public CacheBuilder<K, V> ticker(@NotNull Supplier<Long> ticker) {
Review Comment:
I like this signature, as it is very simple and versatile.
For convenience, should we add a second signature accepting a `Clock`? It
could have a default implementation, which would be the same implementation you
used in `ElasticIndexStatistics`. I expect we will have clock instances around
in quite a few other places? This is just a suggestion and 100% optional. 🙂
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java:
##########
@@ -100,7 +89,7 @@ public class ElasticIndexStatistics implements
IndexStatistics {
*/
@Override
public int numDocs() {
- return countCache.getUnchecked(new
StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()));
+ return countCache.get(new StatsRequestDescriptor(elasticConnection,
indexDefinition.getIndexAlias()));
Review Comment:
I don't know if `new StatsRequestDescriptor(elasticConnection,
indexDefinition.getIndexAlias())` is constant, i.e. if the return value of
`getIndexAlias()` ever changes?
If it is constant, wouldn't it make sense to create a single instance in the
constructor and store it in a field? This would avoid creating the instance
over and over. Of course, I don't know how frequently this happens, so it may
make no practical difference.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java:
##########
@@ -208,25 +191,13 @@ private Long getCacheRefreshSeconds() {
return Long.getLong(REFRESH_SECONDS, REFRESH_SECONDS_DEFAULT);
}
- static class CountCacheLoader extends CacheLoader<StatsRequestDescriptor,
Integer> {
+ static class CountCacheLoader implements
CacheLoader<StatsRequestDescriptor, Integer> {
@Override
public @NotNull Integer load(@NotNull StatsRequestDescriptor
countRequestDescriptor) throws IOException {
return count(countRequestDescriptor);
}
- @Override
- public @NotNull ListenableFuture<Integer> reload(@NotNull
StatsRequestDescriptor crd, @NotNull Integer oldValue) {
- CompletableFuture<Integer> task = CompletableFuture.supplyAsync(()
-> {
- try {
- return count(crd);
- } catch (IOException e) {
- throw new CompletionException(e);
- }
- }, REFRESH_EXECUTOR);
- return FutureConverter.toListenableFuture(task);
- }
-
Review Comment:
Do we (currently) allow toggling between the Caffeine and Guava
implementations?
If that's the case, we would need to somehow keep the "reload" functionality
for Guava, right?
--
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]
