Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getAllPages() returns all pages after cache ttl has expired #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

getAllPages() returns all pages after cache ttl has expired #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions jspwiki-cache/src/main/java/org/apache/wiki/cache/CachingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Licensed to the Apache Software Foundation (ASF) under one
package org.apache.wiki.cache;


import net.sf.ehcache.event.CacheEventListener;
import org.apache.wiki.util.CheckedSupplier;

import java.io.Serializable;
Expand Down Expand Up @@ -121,4 +122,12 @@ public interface CachingManager {
*/
void remove( String cacheName, Serializable key );

/**
* Register a listener to a cache
*
* @param cacheName The cache to which the listener should be registered
* @param listener cache listener to register
* @return true if the listener is being added and was not already added
*/
boolean registerListener( String cacheName, CacheEventListener listener );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AlbrechtStriffler, thanks for looking into this!

The CachingManager interface shouldn't include any ehcache specific classes, in order to allow other cache-framework implementations. In order to allow the listener registration, would passing an AtomicBoolean (the private volatile booleans from this PR) instead of the CacheEventListener be enough?

Also, as there's the remote possibility of other CachingManager custom implementations, should this method be a default one with an empty implementation? (Not really sure which would be the correct answer, I'd be surprised if indeed there is someone out there with a custom CachingManager, I'll let you decide on this one)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, your are right, that CacheEventListener is to specific for the CachingManager. But I think a method like updateAllRequestedFlag(AtomicBoolean flag) is also a bit to specific, no? It is a random implementation/optimization detail of the Caching[Attachment]Provider the CachingManager Interface shouldn't have to know about.

How about we add a generic CacheEvent for jspwiki, that is probably a bit simpler than the one from EhCache and when registering to the specific CachingManager, like the EhcacheCachingManager, we can wrap and delegate to that JSPWikiCacheEvent with the Ehcache (or other) specific implementation of the CacheEvent.
Regarding the method attributes in CacheEvent, instead of the Ehcache we could use 'String cacheName' and instead of Element we could use 'Serializable name' and 'T or Object object'

If you are okay with this, I'm happy to adapt the pull request accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

I thought of throwing an event too, but given that one class is a direct dependency of the other, it seemed too much indirection to me. I agree that updateAllRequestedFlag is also too specific. May be something like registerListener( String cacheName, String listener, T... args)? listener could be used to decide which kind of listener build for a given cacheName, with all the required data passed through args. WDYT?

Either going the events way or going through the registerListener method would fine to me, my main concern is that EhCache classes should not appear outside EhcacheCachingManager

cheers,
juan pablo

}
11 changes: 11 additions & 0 deletions jspwiki-cache/src/main/java/org/apache/wiki/cache/EhcacheCachingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Licensed to the Apache Software Foundation (ASF) under one
import net.sf.ehcache.Cache;
import net.sf.ehcache.CacheManager;
import net.sf.ehcache.Element;
import net.sf.ehcache.event.CacheEventListener;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.wiki.api.core.Engine;
Expand Down Expand Up @@ -159,6 +160,16 @@ public void remove( final String cacheName, final Serializable key ) {
}
}

@Override
public boolean registerListener( final String cacheName, final CacheEventListener listener ) {
if (enabled(cacheName)) {
return cacheMap.get(cacheName).getCacheEventNotificationService().registerListener(listener);
}
return false;
}



boolean keyAndCacheAreNotNull( final String cacheName, final Serializable key ) {
return enabled( cacheName ) && key != null;
}
Expand Down
13 changes: 11 additions & 2 deletions jspwiki-main/src/main/java/org/apache/wiki/providers/CachingAttachmentProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Licensed to the Apache Software Foundation (ASF) under one
*/
package org.apache.wiki.providers;

import net.sf.ehcache.Ehcache;
import net.sf.ehcache.Element;
import net.sf.ehcache.event.CacheEventListenerAdapter;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.wiki.api.core.Attachment;
Expand Down Expand Up @@ -59,7 +62,7 @@ public class CachingAttachmentProvider implements AttachmentProvider {

private AttachmentProvider provider;
private CachingManager cachingManager;
private boolean allRequested;
private volatile boolean allRequested;
private final AtomicLong attachments = new AtomicLong( 0L );

/**
Expand All @@ -68,7 +71,13 @@ public class CachingAttachmentProvider implements AttachmentProvider {
@Override
public void initialize( final Engine engine, final Properties properties ) throws NoRequiredPropertyException, IOException {
LOG.info( "Initing CachingAttachmentProvider" );
cachingManager = engine.getManager( CachingManager.class );
cachingManager = engine.getManager( CachingManager.class );
cachingManager.registerListener( CachingManager.CACHE_PAGES, new CacheEventListenerAdapter() {
@Override
public void notifyElementExpired(Ehcache cache, Element element) {
allRequested = false; // signal that the cache no longer contains all elements...
}
});

// Find and initialize real provider.
final String classname;
Expand Down
11 changes: 10 additions & 1 deletion jspwiki-main/src/main/java/org/apache/wiki/providers/CachingProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Licensed to the Apache Software Foundation (ASF) under one
*/
package org.apache.wiki.providers;

import net.sf.ehcache.Ehcache;
import net.sf.ehcache.Element;
import net.sf.ehcache.event.CacheEventListenerAdapter;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.wiki.api.core.Context;
Expand Down Expand Up @@ -67,7 +70,7 @@ public class CachingProvider implements PageProvider {
private PageProvider provider;
private Engine engine;

private boolean allRequested;
private volatile boolean allRequested;
private final AtomicLong pages = new AtomicLong( 0L );

/**
Expand All @@ -80,6 +83,12 @@ public void initialize( final Engine engine, final Properties properties ) throw
// engine is used for getting the search engine
this.engine = engine;
cachingManager = this.engine.getManager( CachingManager.class );
cachingManager.registerListener( CachingManager.CACHE_PAGES, new CacheEventListenerAdapter() {
@Override
public void notifyElementExpired(Ehcache cache, Element element) {
allRequested = false; // signal that the cache no longer contains all elements...
}
});

// Find and initialize real provider.
final String classname;
Expand Down