Stop memoizing fetch refSpecs
Change I29507376e introduced the memoization fetch refsSpecs.
This was intended to avoid expensive, repeated delta computations for
fetches without an explicit delta to fetch (for example via the
`pull-replication start` SSH command), however the memoization was
introduced for _both_ delta and non-delta fetches.
The problem with this approach is that it doesn't account for the fact
that delta is a mutable entity: `FetchOne` could be rescheduled for
whatever reason, consolidating new refs into a new delta.
If `FetchOne` tried to replicate again based on the new delta refs, the
memoizing would have caused the return of the previous delta value,
causing the effective loss of the refs to replicate.
Remove memoization altogether. This trade offs the possible expensive
computation of expanding non-delta fetches multiple times, in case of a
reschedule, in favour of accuracy of in fetching only the relevant refs.
Bug: Issue 337338030
Change-Id: I06442a66896205258b2b0411f74855465c333864
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
index 66aa8d4..bad77d3 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
@@ -17,7 +17,6 @@
import static com.googlesource.gerrit.plugins.replication.pull.PullReplicationLogger.repLog;
import static java.util.concurrent.TimeUnit.NANOSECONDS;
-import com.google.common.base.Suppliers;
import com.google.common.base.Throwables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
@@ -50,7 +49,6 @@
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.function.Supplier;
import java.util.stream.Collectors;
import org.eclipse.jgit.errors.NoRemoteRepositoryException;
import org.eclipse.jgit.errors.NotSupportedException;
@@ -109,16 +107,6 @@
private DynamicItem<ReplicationFetchFilter> replicationFetchFilter;
private boolean succeeded;
- private final Supplier<List<RefSpec>> fetchRefSpecsSupplier =
- Suppliers.memoize(
- () -> {
- try {
- return computeFetchRefSpecs();
- } catch (IOException e) {
- throw new RuntimeException("Could not compute refs specs to fetch", e);
- }
- });
-
@Inject
FetchOne(
GitRepositoryManager grm,
@@ -457,7 +445,7 @@
private List<RefSpec> runImpl() throws IOException {
Fetch fetch = fetchFactory.create(taskIdHex, uri, git);
- List<RefSpec> fetchRefSpecs = fetchRefSpecsSupplier.get();
+ List<RefSpec> fetchRefSpecs = getFetchRefSpecs();
try {
updateStates(fetch.fetch(fetchRefSpecs));
@@ -498,28 +486,37 @@
*
* @return The list of refSpecs to fetch
*/
- public List<RefSpec> getFetchRefSpecs() {
- return fetchRefSpecsSupplier.get();
- }
-
- private List<RefSpec> computeFetchRefSpecs() throws IOException {
+ public List<RefSpec> getFetchRefSpecs() throws IOException {
List<RefSpec> configRefSpecs = config.getFetchRefSpecs();
if (delta.isEmpty() && replicationFetchFilter().isEmpty()) {
return configRefSpecs;
}
- return runRefsFilter(computeDelta(configRefSpecs)).stream()
+ return runRefsFilter(computeDeltaIfNeeded()).stream()
.map(ref -> refToFetchRefSpec(ref, configRefSpecs))
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
}
- private Set<String> computeDelta(List<RefSpec> configRefSpecs) throws IOException {
+ public List<RefSpec> safeGetFetchRefSpecs() {
+ try {
+ return getFetchRefSpecs();
+ } catch (IOException e) {
+ repLog.error("[{}] Error when evaluating refsSpecs: {}", taskIdHex, e.getMessage());
+ return Collections.emptyList();
+ }
+ }
+
+ private Set<String> computeDeltaIfNeeded() throws IOException {
if (!delta.isEmpty()) {
return delta;
}
+ return staleOrMissingLocalRefs();
+ }
+
+ private Set<String> staleOrMissingLocalRefs() throws IOException {
Map<String, Ref> localRefsMap = fetchRefsDatabase.getLocalRefsMap(git);
Map<String, Ref> remoteRefsMap = fetchRefsDatabase.getRemoteRefsMap(git, uri);
@@ -527,7 +524,7 @@
.filter(
srcRef -> {
// that match our configured refSpecs
- return refToFetchRefSpec(srcRef, configRefSpecs)
+ return refToFetchRefSpec(srcRef, config.getFetchRefSpecs())
.flatMap(
spec ->
shouldBeFetched(srcRef, localRefsMap, remoteRefsMap)
@@ -679,6 +676,11 @@
@Override
public boolean hasSucceeded() {
- return succeeded || getFetchRefSpecs().isEmpty();
+ try {
+ return succeeded || getFetchRefSpecs().isEmpty();
+ } catch (IOException e) {
+ repLog.error("[{}] Error when evaluating refsSpecs: {}", taskIdHex, e.getMessage());
+ return false;
+ }
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchCommand.java
index 47d5391..3227698 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchCommand.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/FetchCommand.java
@@ -113,7 +113,7 @@
} else {
Optional<FetchOne> maybeFetch =
source.get().fetchSync(name, refsNames, source.get().getURI(name), apiRequestMetrics);
- if (maybeFetch.map(FetchOne::getFetchRefSpecs).filter(List::isEmpty).isPresent()) {
+ if (maybeFetch.map(FetchOne::safeGetFetchRefSpecs).filter(List::isEmpty).isPresent()) {
fetchStateLog.warn(
String.format(
"[%s] Nothing to fetch, ref-specs is empty", maybeFetch.get().getTaskIdHex()));
@@ -140,7 +140,7 @@
}
private TransportException newTransportException(FetchOne fetchOne) {
- List<RefSpec> fetchRefSpecs = fetchOne.getFetchRefSpecs();
+ List<RefSpec> fetchRefSpecs = fetchOne.safeGetFetchRefSpecs();
String combinedErrorMessage =
fetchOne.getFetchFailures().stream()
.map(TransportException::getMessage)
