Skip to content

Commit

Permalink
Fix inconcistencies with caching GeoServer config infos
Browse files Browse the repository at this point in the history 
Simplify `CachingGeoServerFacade` making it evict the
whole in-memory cache upon each change or incoming
event.

Trying to maintain too much logic to optimize per-object caching,
specially for workspace `ServiceInfo` and `SettingsInfo` is too
error prone and a maintenance burden.

Evicting the config cache is not a problem while changes are being
performed, and during normal operation with no config changes it
just serves its purpose.
  • Loading branch information
@groldan
groldan committed Oct 26, 2024
1 parent 314c735 commit 858f283
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 269 deletions.
213 changes: 26 additions & 187 deletions ...catalog/cache/src/main/java/org/geoserver/cloud/catalog/cache/CachingGeoServerFacade.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,22 @@
import lombok.NonNull;
import lombok.extern.slf4j.Slf4j;

import org.geoserver.catalog.Info;
import org.geoserver.catalog.WorkspaceInfo;
import org.geoserver.catalog.impl.ResolvingProxy;
import org.geoserver.cloud.event.UpdateSequenceEvent;
import org.geoserver.cloud.event.catalog.CatalogInfoRemoved;
import org.geoserver.cloud.event.config.GeoServerInfoModified;
import org.geoserver.cloud.event.config.GeoServerInfoSet;
import org.geoserver.cloud.event.config.LoggingInfoModified;
import org.geoserver.cloud.event.config.LoggingInfoSet;
import org.geoserver.cloud.event.config.ServiceAdded;
import org.geoserver.cloud.event.config.ServiceModified;
import org.geoserver.cloud.event.config.ServiceRemoved;
import org.geoserver.cloud.event.config.SettingsAdded;
import org.geoserver.cloud.event.config.SettingsModified;
import org.geoserver.cloud.event.config.SettingsRemoved;
import org.geoserver.cloud.event.info.ConfigInfoType;
import org.geoserver.cloud.event.info.InfoEvent;
import org.geoserver.config.GeoServerFacade;
import org.geoserver.config.GeoServerInfo;
import org.geoserver.config.LoggingInfo;
import org.geoserver.config.ServiceInfo;
import org.geoserver.config.SettingsInfo;
import org.geoserver.config.impl.SettingsInfoImpl;
import org.geoserver.config.plugin.forwarding.ForwardingGeoServerFacade;
import org.springframework.cache.Cache;
import org.springframework.cache.Cache.ValueWrapper;
import org.springframework.cache.CacheManager;
import org.springframework.cache.annotation.CacheConfig;
import org.springframework.cache.annotation.CacheEvict;
import org.springframework.cache.annotation.Cacheable;
import org.springframework.cache.annotation.Caching;
import org.springframework.context.event.EventListener;

import java.util.Optional;

import javax.annotation.Nullable;

/** */
Expand Down Expand Up @@ -70,165 +51,20 @@ public CachingGeoServerFacade(@NonNull GeoServerFacade facade, @NonNull Cache ca

////// Event handling ///////

/** Evicts the {@link GeoServerInfo global config} upon any update sequence modifying event. */
@EventListener(classes = UpdateSequenceEvent.class)
void onUpdateSequenceEvent(UpdateSequenceEvent event) {
evictGlobal()
.ifPresent(
evicted ->
log.debug(
"evicted global config with updatesequence {} upon event carrying update sequence {}",
evicted.getUpdateSequence(),
event.getUpdateSequence()));
}

@EventListener(classes = GeoServerInfoModified.class)
void onGeoServerInfoModifyEvent(GeoServerInfoModified event) {
evictConfigEntry(event);
}

@EventListener(classes = LoggingInfoModified.class)
void onLoggingInfoModifyEvent(LoggingInfoModified event) {
evictConfigEntry(event);
}

@EventListener(classes = ServiceAdded.class)
void onServiceInfoAddEvent(ServiceAdded event) {
evictConfigEntry(event);
}

@EventListener(classes = ServiceModified.class)
void onServiceInfoModifyEvent(ServiceModified event) {
evictConfigEntry(event);
}

@EventListener(classes = ServiceRemoved.class)
void onServiceInfoRemoveEvent(ServiceRemoved event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsAdded.class)
void onSettingsInfoAddEvent(SettingsAdded event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsModified.class)
void onSettingsInfoModifyEvent(SettingsModified event) {
evictConfigEntry(event);
}

@EventListener(classes = SettingsRemoved.class)
void onSettingsInfoRemoveEvent(SettingsRemoved event) {
evictConfigEntry(event);
}

@EventListener(classes = GeoServerInfoSet.class)
void onGeoServerInfoSetEvent(GeoServerInfoSet event) {
evictConfigEntry(event);
}

@EventListener(classes = LoggingInfoSet.class)
void onLoggingInfoSetEvent(LoggingInfoSet event) {
evictConfigEntry(event);
}

/**
* Can't know which {@link ServiceInfo} are attached to the workspace when its removed, so it
* invalidates all cache entries, possibly asynchronously, as per {@link Cache#clear()}
* Clears the whole config cache upon any {@link UpdateSequenceEvent}.
*
* <p>As opposed to the other event handler methods, this one evicts upon both {@link
* InfoEvent#isRemote() remote} and {@link InfoEvent#isLocal() local} events.
* <p>{@link UpdateSequenceEvent} is the root event for the ones that change something in the
* catalog or the configuration.
*/
@EventListener(classes = CatalogInfoRemoved.class)
void onWorkspaceRemoved(CatalogInfoRemoved event) {
if (event.getObjectType() == ConfigInfoType.WORKSPACE) {
cache.clear();
}
}

public void evictConfigEntry(InfoEvent event) {
if (!event.isRemote()) return;

String objectId = event.getObjectId();
ConfigInfoType infoType = event.getObjectType();
Info info;
if (event instanceof SettingsModified se) {
info = mockSettings(se.getObjectId(), se.getWorkspaceId());
} else if (event instanceof SettingsRemoved se) {
info = mockSettings(se.getObjectId(), se.getWorkspaceId());
} else {
info = ResolvingProxy.create(objectId, infoType.getType());
}

if (evict(info)) {
log.debug("Evicted config cache entry due to {}", event);
} else {
log.trace("Remote event resulted in no cache eviction: {}", event);
@EventListener(classes = UpdateSequenceEvent.class)
void onUpdateSequenceEvent(UpdateSequenceEvent event) {
if (event.isRemote()) {
evictAll();
}
}

private SettingsInfo mockSettings(@NonNull String objectId, String workspaceId) {
var s = new SettingsInfoImpl();
s.setId(objectId);
s.setWorkspace(ResolvingProxy.create(workspaceId, WorkspaceInfo.class));
return s;
}

////// Cache manipulation functions ///////

Optional<GeoServerInfo> evictGlobal() {
Optional<GeoServerInfo> ret =
Optional.ofNullable(cache.get(GEOSERVERINFO_KEY))
.map(ValueWrapper::get)
.map(GeoServerInfo.class::cast);
cache.evict(GEOSERVERINFO_KEY);
return ret;
}

boolean evict(Info info) {
log.debug("Evict cache entry for {}", info.getId());
if (info instanceof GeoServerInfo) {
return evictGlobal().isPresent();
}
if (info instanceof LoggingInfo) {
return cache.evictIfPresent(LOGGINGINFO_KEY);
}
if (info instanceof SettingsInfo settings) {
return evict(settings);
}
if (info instanceof ServiceInfo service) {
return evict(service);
}
return false;
}

private boolean evict(SettingsInfo settings) {
String id = settings.getId();
boolean evicted = cache.evictIfPresent(id);
WorkspaceInfo workspace = settings.getWorkspace();
if (workspace != null) {
Object wsKey = CachingGeoServerFacade.settingsKey(workspace);
evicted |= cache.evictIfPresent(wsKey);
}
return evicted;
}

private boolean evict(ServiceInfo service) {
Object idKey = CachingGeoServerFacade.serviceByIdKey(service.getId());
ValueWrapper cachedValue = cache.get(idKey);
if (cachedValue == null) return false;
ServiceInfo cached = (ServiceInfo) cachedValue.get();
cache.evict(idKey);
if (cached != null) {
WorkspaceInfo ws = cached.getWorkspace();
Object nameKey = CachingGeoServerFacade.serviceByNameKey(ws, cached.getName());
Object typeKey = CachingGeoServerFacade.serviceByTypeKey(ws, cached.getClass());
cache.evict(nameKey);
cache.evict(typeKey);
}
return true;
}

<T extends ServiceInfo> T cachePutIncludeNull(
@NonNull Object key, @NonNull Cache cache, T service) {

Expand Down Expand Up @@ -263,13 +99,12 @@ public GeoServerInfo getGlobal() {
}

@Override
@CacheEvict(key = "'" + GEOSERVERINFO_KEY + "'", beforeInvocation = true)
@CacheEvict(allEntries = true)
public void setGlobal(GeoServerInfo global) {
super.setGlobal(global);
}

@Override
@CacheEvict(key = "'" + GEOSERVERINFO_KEY + "'", beforeInvocation = true)
public void save(GeoServerInfo geoServer) {
super.save(geoServer);
}
Expand All @@ -292,21 +127,19 @@ public SettingsInfo getSettings(WorkspaceInfo workspace) {
}

@Override
@Caching(
evict = {
@CacheEvict(key = "#settings.id", beforeInvocation = true),
@CacheEvict(key = "'settings@' + #settings.workspace.id", beforeInvocation = true)
})
@CacheEvict(allEntries = true)
public void add(SettingsInfo settings) {
super.add(settings);
}

@Override
@CacheEvict(allEntries = true)
public void save(SettingsInfo settings) {
super.save(settings);
}

@Override
@Caching(
evict = {
@CacheEvict(key = "#settings.id", beforeInvocation = true),
@CacheEvict(key = "'settings@' + #settings.workspace.id", beforeInvocation = true)
})
@CacheEvict(allEntries = true)
public void remove(SettingsInfo settings) {
super.remove(settings);
}
Expand All @@ -318,26 +151,32 @@ public LoggingInfo getLogging() {
}

@Override
@CacheEvict(key = "'" + LOGGINGINFO_KEY + "'", beforeInvocation = true)
@CacheEvict(allEntries = true)
public void setLogging(LoggingInfo logging) {
super.setLogging(logging);
}

@Override
@CacheEvict(key = "'" + LOGGINGINFO_KEY + "'", beforeInvocation = true)
@CacheEvict(allEntries = true)
public void save(LoggingInfo logging) {
super.save(logging);
}

@Override
@CacheEvict(allEntries = true)
public void add(ServiceInfo service) {
super.add(service);
}

@Override
@CacheEvict(allEntries = true)
public void remove(ServiceInfo service) {
evict(service);
super.remove(service);
}

@Override
@CacheEvict(allEntries = true)
public void save(ServiceInfo service) {
evict(service);
super.save(service);
}

Expand Down
Loading

0 comments on commit 858f283

Please sign in to comment.