TasksStorage: Replace delete() with start()+finish() in tests
During the merge of stable-3.0, the delete() method was re-added [1]
to fix a test that relied upon it. Instead, that test should have been
updated to use the start() and finish() methods that are part of the
public API for ReplicationTasksStorage.
To facilitate that update, modify the start(), reset(), and finish()
methods to not require a PushOne object. These methods can instead
use a new interface that PushOne already can implement without
functional changes. This interface avoids duplicate code in
ReplicationTasksStorage.
[1] https://gerrit-review.googlesource.com/c/plugins/replication/+/273660/-1..4/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
Change-Id: Id82d2ec3125075832134c8dbe56b342e5a874bbc
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
index 634f44d..b53bf69 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
@@ -84,7 +84,7 @@
* <p>Instance members are protected by the lock within PushQueue. Callers must take that lock to
* ensure they are working with a current view of the object.
*/
-class PushOne implements ProjectRunnable, CanceledWhileRunning {
+class PushOne implements ProjectRunnable, CanceledWhileRunning, UriUpdates {
private final ReplicationStateListener stateLog;
static final String ALL_REFS = "..all..";
static final String ID_MDC_KEY = "pushOneId";
@@ -230,7 +230,8 @@
return canceled || canceledWhileRunning.get();
}
- URIish getURI() {
+ @Override
+ public URIish getURI() {
return uri;
}
@@ -244,7 +245,8 @@
}
}
- Set<String> getRefs() {
+ @Override
+ public Set<String> getRefs() {
return pushAllRefs ? Sets.newHashSet(ALL_REFS) : delta;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
index 370d48b..d697d11 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -68,8 +68,12 @@
public final String uri;
public final String remote;
- public ReplicateRefUpdate(PushOne push, String ref) {
- this(push.getProjectNameKey().get(), ref, push.getURI(), push.getRemoteName());
+ public ReplicateRefUpdate(UriUpdates uriUpdates, String ref) {
+ this(
+ uriUpdates.getProjectNameKey().get(),
+ ref,
+ uriUpdates.getURI(),
+ uriUpdates.getRemoteName());
}
public ReplicateRefUpdate(String project, String ref, URIish uri, String remote) {
@@ -114,20 +118,15 @@
this.disableDeleteForTesting = deleteDisabled;
}
- @VisibleForTesting
- public void delete(ReplicateRefUpdate r) {
- new Task(r).delete();
- }
-
- public synchronized void start(PushOne push) {
- for (String ref : push.getRefs()) {
- new Task(new ReplicateRefUpdate(push, ref)).start();
+ public synchronized void start(UriUpdates uriUpdates) {
+ for (String ref : uriUpdates.getRefs()) {
+ new Task(new ReplicateRefUpdate(uriUpdates, ref)).start();
}
}
- public synchronized void reset(PushOne push) {
- for (String ref : push.getRefs()) {
- new Task(new ReplicateRefUpdate(push, ref)).reset();
+ public synchronized void reset(UriUpdates uriUpdates) {
+ for (String ref : uriUpdates.getRefs()) {
+ new Task(new ReplicateRefUpdate(uriUpdates, ref)).reset();
}
}
@@ -137,9 +136,9 @@
}
}
- public synchronized void finish(PushOne push) {
- for (String ref : push.getRefs()) {
- new Task(new ReplicateRefUpdate(push, ref)).finish();
+ public synchronized void finish(UriUpdates uriUpdates) {
+ for (String ref : uriUpdates.getRefs()) {
+ new Task(new ReplicateRefUpdate(uriUpdates, ref)).finish();
}
}
@@ -261,15 +260,6 @@
}
}
- public void delete() {
- try {
- Files.deleteIfExists(waiting);
- Files.deleteIfExists(running);
- } catch (IOException e) {
- logger.atSevere().withCause(e).log("Error while deleting task %s", taskKey);
- }
- }
-
private void rename(Path from, Path to) {
try {
logger.atFine().log("RENAME %s to %s %s", from, to, updateLog());
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/UriUpdates.java b/src/main/java/com/googlesource/gerrit/plugins/replication/UriUpdates.java
new file mode 100644
index 0000000..d6d6adf
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/UriUpdates.java
@@ -0,0 +1,30 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.replication;
+
+import com.google.gerrit.entities.Project;
+import java.util.Set;
+import org.eclipse.jgit.transport.URIish;
+
+/** A data container for updates to a single URI */
+public interface UriUpdates {
+ Project.NameKey getProjectNameKey();
+
+ URIish getURI();
+
+ String getRemoteName();
+
+ Set<String> getRefs();
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
index 31cd75d..1060780 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -37,6 +37,7 @@
import com.google.inject.Key;
import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
import java.io.IOException;
+import java.net.URISyntaxException;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
@@ -449,7 +450,7 @@
}
@Test
- public void shouldFirePendingOnlyToStoredUri() throws Exception {
+ public void shouldFirePendingOnlyToRemainingUris() throws Exception {
String suffix1 = "replica1";
String suffix2 = "replica2";
Project.NameKey target1 = createTestProject(project + suffix1);
@@ -463,7 +464,16 @@
String changeRef = createChange().getPatchSet().refName();
tasksStorage.disableDeleteForTesting(false);
- changeReplicationTasksForRemote(changeRef, remote1).forEach(tasksStorage::delete);
+ changeReplicationTasksForRemote(changeRef, remote1)
+ .forEach(
+ (update) -> {
+ try {
+ UriUpdates uriUpdates = TestUriUpdates.create(update);
+ tasksStorage.start(uriUpdates);
+ tasksStorage.finish(uriUpdates);
+ } catch (URISyntaxException e) {
+ }
+ });
tasksStorage.disableDeleteForTesting(true);
setReplicationDestination(remote1, suffix1, ALL_PROJECTS);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
index 38d5421..5b8a05a 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -42,12 +42,14 @@
protected ReplicationTasksStorage storage;
protected FileSystem fileSystem;
protected Path storageSite;
+ protected UriUpdates uriUpdates;
@Before
public void setUp() throws Exception {
fileSystem = Jimfs.newFileSystem(Configuration.unix());
storageSite = fileSystem.getPath("replication_site");
storage = new ReplicationTasksStorage(storageSite);
+ uriUpdates = TestUriUpdates.create(REF_UPDATE);
}
@After
@@ -67,9 +69,10 @@
}
@Test
- public void canDeletePersistedUpdate() throws Exception {
+ public void canFinishPersistedUpdate() throws Exception {
storage.create(REF_UPDATE);
- storage.delete(REF_UPDATE);
+ storage.start(uriUpdates);
+ storage.finish(uriUpdates);
assertThat(storage.list()).isEmpty();
}
@@ -84,7 +87,8 @@
assertContainsExactly(storage, REF_UPDATE);
assertContainsExactly(persistedView, REF_UPDATE);
- storage.delete(REF_UPDATE);
+ storage.start(uriUpdates);
+ storage.finish(uriUpdates);
assertThat(storage.list()).isEmpty();
assertThat(persistedView.list()).isEmpty();
}
@@ -113,20 +117,23 @@
}
@Test
- public void canDeleteDifferentUris() throws Exception {
+ public void canFinishDifferentUris() throws Exception {
ReplicateRefUpdate updateB =
new ReplicateRefUpdate(
PROJECT,
REF,
getUrish("ssh://example.com/" + PROJECT + ".git"), // uses ssh not http
REMOTE);
+ UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);
storage.create(REF_UPDATE);
storage.create(updateB);
+ storage.start(uriUpdates);
+ storage.start(uriUpdatesB);
- storage.delete(REF_UPDATE);
+ storage.finish(uriUpdates);
assertContainsExactly(storage, updateB);
- storage.delete(updateB);
+ storage.finish(uriUpdatesB);
assertThat(storage.list()).isEmpty();
}
@@ -158,47 +165,55 @@
}
@Test
- public void canDeleteMulipleRefsForSameUri() throws Exception {
- ReplicateRefUpdate refA = new ReplicateRefUpdate(PROJECT, "refA", URISH, REMOTE);
- ReplicateRefUpdate refB = new ReplicateRefUpdate(PROJECT, "refB", URISH, REMOTE);
- storage.create(refA);
- storage.create(refB);
+ public void canFinishMulipleRefsForSameUri() throws Exception {
+ ReplicateRefUpdate refUpdateA = new ReplicateRefUpdate(PROJECT, "refA", URISH, REMOTE);
+ ReplicateRefUpdate refUpdateB = new ReplicateRefUpdate(PROJECT, "refB", URISH, REMOTE);
+ UriUpdates uriUpdatesA = TestUriUpdates.create(refUpdateA);
+ UriUpdates uriUpdatesB = TestUriUpdates.create(refUpdateB);
+ storage.create(refUpdateA);
+ storage.create(refUpdateB);
+ storage.start(uriUpdatesA);
+ storage.start(uriUpdatesB);
- storage.delete(refA);
- assertContainsExactly(storage, refB);
+ storage.finish(uriUpdatesA);
+ assertContainsExactly(storage, refUpdateB);
- storage.delete(refB);
+ storage.finish(uriUpdatesB);
assertThat(storage.list()).isEmpty();
}
@Test(expected = Test.None.class /* no exception expected */)
- public void illegalDeleteNonPersistedIsGraceful() throws Exception {
- storage.delete(REF_UPDATE);
+ public void illegalFinishNonPersistedIsGraceful() throws Exception {
+ storage.finish(uriUpdates);
}
@Test(expected = Test.None.class /* no exception expected */)
- public void illegalDoubleDeleteIsGraceful() throws Exception {
+ public void illegalDoubleFinishIsGraceful() throws Exception {
storage.create(REF_UPDATE);
- storage.delete(REF_UPDATE);
+ storage.start(uriUpdates);
+ storage.finish(uriUpdates);
- storage.delete(REF_UPDATE);
+ storage.finish(uriUpdates);
}
@Test(expected = Test.None.class /* no exception expected */)
- public void illegalDoubleDeleteDifferentUriIsGraceful() throws Exception {
+ public void illegalDoubleFinishDifferentUriIsGraceful() throws Exception {
ReplicateRefUpdate updateB =
new ReplicateRefUpdate(
PROJECT,
REF,
getUrish("ssh://example.com/" + PROJECT + ".git"), // uses ssh not http
REMOTE);
+ UriUpdates uriUpdatesB = TestUriUpdates.create(updateB);
storage.create(REF_UPDATE);
storage.create(updateB);
- storage.delete(REF_UPDATE);
- storage.delete(updateB);
+ storage.start(uriUpdates);
+ storage.start(uriUpdatesB);
+ storage.finish(uriUpdates);
+ storage.finish(uriUpdatesB);
- storage.delete(REF_UPDATE);
- storage.delete(updateB);
+ storage.finish(uriUpdates);
+ storage.finish(uriUpdatesB);
assertThat(storage.list()).isEmpty();
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
new file mode 100644
index 0000000..2fd3ee3
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
@@ -0,0 +1,51 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.replication;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.entities.Project;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import java.net.URISyntaxException;
+import java.util.Collections;
+import java.util.Set;
+import org.eclipse.jgit.transport.URIish;
+
+@AutoValue
+public abstract class TestUriUpdates implements UriUpdates {
+ public static TestUriUpdates create(ReplicateRefUpdate update) throws URISyntaxException {
+ return create(
+ Project.nameKey(update.project),
+ new URIish(update.uri),
+ update.remote,
+ Collections.singleton(update.ref));
+ }
+
+ public static TestUriUpdates create(
+ Project.NameKey project, URIish uri, String remote, Set<String> refs) {
+ return new AutoValue_TestUriUpdates(project, uri, remote, refs);
+ }
+
+ @Override
+ public abstract Project.NameKey getProjectNameKey();
+
+ @Override
+ public abstract URIish getURI();
+
+ @Override
+ public abstract String getRemoteName();
+
+ @Override
+ public abstract Set<String> getRefs();
+}
