Do not cache negative results from open changes / timestamp lookups When the open changes and ts caches are failing to load their respective data from the repository, the caching of the failure may have disruptive consequences. If the failure was just the result of an intermediate replication event (e.g. a new patch-set has been been replicated but its associated /meta is still pending) the negative caching would result in making the ref not discoverable. The management of the loading failures from the cache is already coded in the ForProjectWrapper and therefore there is no need to duplicate a similar logic in the caches as well. Bug: Issue 299486879 Change-Id: Ic8015238d1f61ad6c47ebdbf6519533dbfa7ec04
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java index cfd7a0f..248d347 100644 --- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java +++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/ChangesTsCache.java
@@ -14,10 +14,8 @@ package com.googlesource.gerrit.modules.gitrefsfilter; import com.google.common.cache.CacheLoader; -import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.project.NoSuchChangeException; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Singleton; @@ -38,7 +36,6 @@ @Singleton static class Loader extends CacheLoader<ChangeCacheKey, Long> { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final ChangeNotes.Factory changeNotesFactory; @Inject @@ -48,17 +45,11 @@ @Override public Long load(ChangeCacheKey key) throws Exception { - try { - return changeNotesFactory - .createChecked(key.repo(), key.project(), key.changeId(), key.changeRevision()) - .getChange() - .getLastUpdatedOn() - .getTime(); - } catch (NoSuchChangeException e) { - logger.atFine().withCause(e).log( - "Change %d does not exist: returning zero epoch", key.changeId()); - return 0L; - } + return changeNotesFactory + .createChecked(key.repo(), key.project(), key.changeId(), key.changeRevision()) + .getChange() + .getLastUpdatedOn() + .getTime(); } } }
diff --git a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/OpenChangesCache.java b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/OpenChangesCache.java index 8e1c371..d8dd16b 100644 --- a/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/OpenChangesCache.java +++ b/src/main/java/com/googlesource/gerrit/modules/gitrefsfilter/OpenChangesCache.java
@@ -14,10 +14,8 @@ package com.googlesource.gerrit.modules.gitrefsfilter; import com.google.common.cache.CacheLoader; -import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.project.NoSuchChangeException; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Singleton; @@ -38,7 +36,6 @@ @Singleton static class Loader extends CacheLoader<ChangeCacheKey, Boolean> { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final ChangeNotes.Factory changeNotesFactory; @Inject @@ -48,16 +45,10 @@ @Override public Boolean load(ChangeCacheKey key) throws Exception { - try { - ChangeNotes changeNotes = - changeNotesFactory.createChecked( - key.repo(), key.project(), key.changeId(), key.changeRevision()); - return changeNotes.getChange().getStatus().isOpen(); - } catch (NoSuchChangeException e) { - logger.atFine().withCause(e).log( - "Change %d does not exist: hiding from the advertised refs", key.changeId()); - return false; - } + ChangeNotes changeNotes = + changeNotesFactory.createChecked( + key.repo(), key.project(), key.changeId(), key.changeRevision()); + return changeNotes.getChange().getStatus().isOpen(); } } }