Do not lock refs when not executing a fetch Locking the local refs is needed only when executing a Git fetch, otherwise keep on using the filtering logic without introducing any local lock. Also remove the recursion in FetchOne.runImpl() and replace with a while/loop for avoiding the nested locking. Change-Id: I57475e3f264efa340765dd357ef6b8bc8e61c284
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 a5c66c0..472d64d 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
@@ -73,6 +73,8 @@ public class FetchOne implements ProjectRunnable, CanceledWhileRunning, Completable { private final ReplicationStateListener stateLog; public static final String ALL_REFS = "..all.."; + public static final boolean FILTER_AND_LOCK = true; + public static final boolean FILTER_ONLY = false; static final String ID_KEY = "fetchOneId"; private final DeleteRefCommand deleteRefCommand; @@ -474,44 +476,46 @@ } private List<FetchRefSpec> runImpl() throws IOException { - Fetch fetch = fetchFactory.create(taskIdHex, uri, git); - List<FetchRefSpec> fetchRefSpecs = getFetchRefSpecs(); + while (true) { + Fetch fetch = fetchFactory.create(taskIdHex, uri, git); + List<FetchRefSpec> fetchRefSpecs = getFetchRefSpecs(FILTER_AND_LOCK); - try { - List<FetchRefSpec> toFetch = - fetchRefSpecs.stream().filter(rs -> rs.getSource() != null).toList(); - Set<String> toDelete = refsToDelete(fetchRefSpecs); - updateStates(fetch.fetch(toFetch)); + try { + repLog.info("[{}] Running fetch from {} on {} ...", taskIdHex, uri, fetchRefSpecs); - // JGit doesn't support a fetch of <empty> to a ref (e.g. :refs/to/delete) therefore we have - // manage them separately and remove them one by one. - if (!toDelete.isEmpty()) { - updateStates( - deleteRefCommand.deleteRefsSync(taskIdHex, projectName, toDelete, getRemoteName())); + List<FetchRefSpec> toFetch = + fetchRefSpecs.stream().filter(rs -> rs.getSource() != null).toList(); + Set<String> toDelete = refsToDelete(fetchRefSpecs); + updateStates(fetch.fetch(toFetch)); + + // JGit doesn't support a fetch of <empty> to a ref (e.g. :refs/to/delete) therefore we have + // manage them separately and remove them one by one. + if (!toDelete.isEmpty()) { + updateStates( + deleteRefCommand.deleteRefsSync(taskIdHex, projectName, toDelete, getRemoteName())); + } + return fetchRefSpecs; + } catch (InexistentRefTransportException e) { + String inexistentRef = e.getInexistentRef(); + repLog.info( + "[{}] Remote {} does not have ref {} in replication task, flagging as failed and" + + " removing from the replication task", + taskIdHex, + uri, + inexistentRef); + fetchFailures.add(e); + delta.remove(FetchRefSpec.fromRef(inexistentRef)); + if (delta.isEmpty()) { + repLog.warn("[{}] Empty replication task, skipping.", taskIdHex); + return Collections.emptyList(); + } + } catch (IOException e) { + notifyRefReplicatedIOException(); + throw e; + } finally { + unlockRefSpecs(fetchLocks); } - } catch (InexistentRefTransportException e) { - String inexistentRef = e.getInexistentRef(); - repLog.info( - "[{}] Remote {} does not have ref {} in replication task, flagging as failed and removing" - + " from the replication task", - taskIdHex, - uri, - inexistentRef); - fetchFailures.add(e); - delta.remove(FetchRefSpec.fromRef(inexistentRef)); - if (delta.isEmpty()) { - repLog.warn("[{}] Empty replication task, skipping.", taskIdHex); - return Collections.emptyList(); - } - - runImpl(); - } catch (IOException e) { - notifyRefReplicatedIOException(); - throw e; - } finally { - unlockRefSpecs(fetchLocks); } - return fetchRefSpecs; } @VisibleForTesting @@ -542,9 +546,10 @@ * <p>When {@link FetchOne#delta} is not empty and {@link FetchOne#replicationFetchFilter} was * provided, the filtered refsSpecs are returned. * + * @param lock true if the refs should also be locally locked upon filtering. * @return The list of refSpecs to fetch */ - public List<FetchRefSpec> getFetchRefSpecs() throws IOException { + public List<FetchRefSpec> getFetchRefSpecs(boolean lock) throws IOException { List<FetchRefSpec> configRefSpecs = config.getFetchRefSpecs().stream().map(FetchRefSpec::fromRefSpec).toList(); @@ -552,7 +557,7 @@ return configRefSpecs; } - return runRefsFilter(computeDeltaIfNeeded()).stream() + return runRefsFilter(computeDeltaIfNeeded(), lock).stream() .map(ref -> refToFetchRefSpec(ref, configRefSpecs)) .filter(Optional::isPresent) .map(Optional::get) @@ -580,7 +585,7 @@ public List<FetchRefSpec> safeGetFetchRefSpecs() { try { - return getFetchRefSpecs(); + return getFetchRefSpecs(FILTER_ONLY); } catch (IOException e) { repLog.error("[{}] Error when evaluating refsSpecs: {}", taskIdHex, e.getMessage()); return Collections.emptyList(); @@ -627,15 +632,19 @@ .flatMap(filter -> Optional.ofNullable(filter.get())); } - private Set<FetchRefSpec> runRefsFilter(Set<FetchRefSpec> refs) { + private Set<FetchRefSpec> runRefsFilter(Set<FetchRefSpec> refs, boolean lock) { Set<String> refsNames = refs.stream().map(FetchRefSpec::refName).collect(Collectors.toUnmodifiableSet()); Set<String> filteredRefNames = replicationFetchFilter() .map( f -> { - fetchLocks = f.filterAndLock(this.projectName.get(), refsNames); - return fetchLocks.keySet(); + if (lock) { + fetchLocks = f.filterAndLock(this.projectName.get(), refsNames); + return fetchLocks.keySet(); + } else { + return f.filter(this.projectName.get(), refsNames); + } }) .orElse(refsNames); return refs.stream() @@ -780,7 +789,7 @@ @Override public boolean hasSucceeded() { try { - return succeeded || getFetchRefSpecs().isEmpty(); + return succeeded || getFetchRefSpecs(FILTER_ONLY).isEmpty(); } catch (IOException e) { repLog.error("[{}] Error when evaluating refsSpecs: {}", taskIdHex, e.getMessage()); return false;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java index 223f720..1104ff4 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.replication.pull; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.replication.pull.FetchOne.FILTER_ONLY; import static com.googlesource.gerrit.plugins.replication.pull.FetchOne.refsToDelete; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.anyList; @@ -182,7 +183,7 @@ objectUnderTest.addRefs(refSpecsSetOf(TEST_REF)); objectUnderTest.addRefs(refSpecsSetOf(TEST_DELETE_REF)); - assertThat(objectUnderTest.getFetchRefSpecs()) + assertThat(objectUnderTest.getFetchRefSpecs(FILTER_ONLY)) .isEqualTo(List.of(FetchRefSpec.fromRef(TEST_DELETE_REF))); } @@ -192,7 +193,7 @@ objectUnderTest.addRefs(refSpecsSetOf(TEST_DELETE_REF)); objectUnderTest.addRefs(refSpecsSetOf(TEST_REF)); - assertThat(objectUnderTest.getFetchRefSpecs()) + assertThat(objectUnderTest.getFetchRefSpecs(FetchOne.FILTER_ONLY)) .isEqualTo(List.of(FetchRefSpec.fromRef(TEST_REF + ":" + TEST_REF))); }