jsedding commented on code in PR #68:
URL:
https://github.com/apache/sling-org-apache-sling-servlets-resolver/pull/68#discussion_r3570582767
##########
src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java:
##########
@@ -278,17 +293,33 @@ public Servlet get(final AbstractResourceCollector
context) {
return null;
}
- public void put(final AbstractResourceCollector context, final Servlet
candidate) {
- final Map<AbstractResourceCollector, Servlet> localCache =
this.cache.get();
- if (localCache != null) {
- if (localCache.size() < this.cacheSize) {
- localCache.put(context, candidate);
- } else if (this.logCacheSizeWarning) {
- this.logCacheSizeWarning = false;
- logger.warn(
- "Script cache has reached its limit of {}. You might
want to increase the cache size for the servlet resolver.",
- this.cacheSize);
+ /**
+ * @return the current cache generation, to be passed to {@link #put(long,
AbstractResourceCollector, Servlet)}
+ */
+ public long getGeneration() {
+ return this.generation.get();
Review Comment:
I think this should be guarded by the read-lock, because it is used as a
parameter to `put`, making it conceptually part of the `put`. Granted, the only
"race" I can see results in throwing away a resolution that _could_ have been
cached. I.e. if `getGeneration` is called during a flush, but before generation
is incremented.
##########
src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java:
##########
@@ -278,17 +293,33 @@ public Servlet get(final AbstractResourceCollector
context) {
return null;
}
- public void put(final AbstractResourceCollector context, final Servlet
candidate) {
- final Map<AbstractResourceCollector, Servlet> localCache =
this.cache.get();
- if (localCache != null) {
- if (localCache.size() < this.cacheSize) {
- localCache.put(context, candidate);
- } else if (this.logCacheSizeWarning) {
- this.logCacheSizeWarning = false;
- logger.warn(
- "Script cache has reached its limit of {}. You might
want to increase the cache size for the servlet resolver.",
- this.cacheSize);
+ /**
+ * @return the current cache generation, to be passed to {@link #put(long,
AbstractResourceCollector, Servlet)}
+ */
+ public long getGeneration() {
+ return this.generation.get();
+ }
+
+ /**
+ * Caches the candidate unless the cache has been flushed since {@code
generation} was obtained,
+ * i.e. the resolution that produced the candidate may be based on stale
data.
+ */
+ public void put(final long generation, final AbstractResourceCollector
context, final Servlet candidate) {
Review Comment:
I wonder if `put` could be turned into `computeIfAbsent`, allowing the
generation to be hidden within the `ResolutionCache` as an implementation
detail.
--
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]
