Fix scheduling of tasks using the old storage format
The replication task storage uses the sha-sum of a string generated
from the objects properties as a filename. While scheduling the
task this shasum is computed again. Since the string which is the
basis for computing the shasum changed recently, this computation
results in a different result than the filename of the stored task.
Now the filename is used during scheduling and the shasum is not
recomputed. This allows to schedule tasks still using the old format.
Change-Id: I2dd0965ae0c7b1160f8c0774153f40edfae6c55a
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 9e736ea..3d04cdd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -86,13 +86,22 @@
public static ReplicateRefUpdate create(Path file, Gson gson) throws IOException {
String json = new String(Files.readAllBytes(file), UTF_8);
- return gson.fromJson(json, ReplicateRefUpdate.class);
+ return create(gson.fromJson(json, ReplicateRefUpdate.class), file.getFileName().toString());
}
public static ReplicateRefUpdate create(
String project, Set<String> refs, URIish uri, String remote) {
return new AutoValue_ReplicationTasksStorage_ReplicateRefUpdate(
- project, ImmutableSet.copyOf(refs), uri.toASCIIString(), remote);
+ project,
+ ImmutableSet.copyOf(refs),
+ uri.toASCIIString(),
+ remote,
+ sha1(project, ImmutableSet.copyOf(refs), uri.toASCIIString(), remote));
+ }
+
+ public static ReplicateRefUpdate create(ReplicateRefUpdate u, String filename) {
+ return new AutoValue_ReplicationTasksStorage_ReplicateRefUpdate(
+ u.project(), u.refs(), u.uri(), u.remote(), filename);
}
public abstract String project();
@@ -103,9 +112,11 @@
public abstract String remote();
- public String sha1() {
+ public abstract String sha1();
+
+ private static String sha1(String project, Set<String> refs, String uri, String remote) {
return ReplicationTasksStorage.sha1(
- project() + "\n" + refs().toString() + "\n" + uri() + "\n" + remote())
+ project + "\n" + refs.toString() + "\n" + uri + "\n" + remote)
.name();
}
@@ -377,7 +388,8 @@
}
}
- private boolean rename(Path from, Path to) {
+ @VisibleForTesting
+ boolean rename(Path from, Path to) {
try {
logger.atFine().log("RENAME %s to %s %s", from, to, updateLog());
Files.move(from, to, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageMPTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageMPTest.java
index 6de0b6b..cf5168f 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageMPTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageMPTest.java
@@ -38,6 +38,8 @@
ReplicationTasksStorageTest.getUrish("http://example.com/" + PROJECT + ".git");
protected static final ReplicateRefUpdate REF_UPDATE =
ReplicateRefUpdate.create(PROJECT, Set.of(REF), URISH, REMOTE);
+ protected static final ReplicateRefUpdate STORED_REF_UPDATE =
+ ReplicateRefUpdate.create(REF_UPDATE, REF_UPDATE.sha1());
protected static final UriUpdates URI_UPDATES = getUriUpdates(REF_UPDATE);
protected ReplicationTasksStorage nodeA;
@@ -67,7 +69,7 @@
nodeA.create(REF_UPDATE);
nodeB.create(REF_UPDATE);
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
}
@Test
@@ -75,7 +77,7 @@
nodeA.create(REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -87,10 +89,10 @@
nodeA.start(URI_UPDATES);
nodeA.reset(URI_UPDATES);
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -104,10 +106,10 @@
nodeB.start(URI_UPDATES);
nodeB.reset(URI_UPDATES);
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -122,7 +124,7 @@
nodeB.reset(URI_UPDATES);
nodeA.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeA.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -134,10 +136,10 @@
nodeA.start(URI_UPDATES);
nodeB.recoverAll();
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeA.finish(URI_UPDATES);
// Bug: https://crbug.com/gerrit/12973
@@ -154,10 +156,10 @@
nodeB.recoverAll();
nodeA.finish(URI_UPDATES);
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -170,7 +172,7 @@
nodeB.recoverAll();
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
ReplicationTasksStorageTest.assertNoIncompleteTasks(persistedView);
@@ -183,14 +185,14 @@
public void multipleNodesCanReplicateSameRef() {
nodeA.create(REF_UPDATE);
nodeA.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeA.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
nodeB.create(REF_UPDATE);
nodeB.start(URI_UPDATES);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
nodeB.finish(URI_UPDATES);
assertNoIncompleteTasks(persistedView);
@@ -202,10 +204,10 @@
nodeB.create(REF_UPDATE);
assertThat(nodeA.start(URI_UPDATES)).containsExactly(REF_UPDATE.refs());
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
assertThat(nodeB.start(URI_UPDATES)).isEmpty();
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
}
public static UriUpdates getUriUpdates(ReplicationTasksStorage.ReplicateRefUpdate refUpdate) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTaskTest.java
index da27bc1..43c0b3e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTaskTest.java
@@ -15,11 +15,13 @@
package com.googlesource.gerrit.plugins.replication;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import com.google.common.collect.ImmutableSet;
+import com.google.common.hash.Hashing;
import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import com.google.gson.Gson;
@@ -31,7 +33,9 @@
import java.net.URISyntaxException;
import java.nio.file.FileSystem;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.util.Set;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.transport.URIish;
import org.junit.After;
import org.junit.Before;
@@ -410,11 +414,16 @@
ImmutableSet.of("ref1", "ref2"),
new URIish("git://host1/someRepo.git"),
"someRemote");
- assertEquals(
+ ReplicateRefUpdate restoredUpdate2 =
gson.fromJson(
"{\"project\":\"someProject\",\"refs\":[\"ref1\",\"ref2\"],\"uri\":\"git://host1/someRepo.git\",\"remote\":\"someRemote\"}",
- ReplicateRefUpdate.class),
- update2);
+ ReplicateRefUpdate.class);
+ // ReplicateRefUpdate.sha() might be different for the compared objects, since
+ // the order of refs() might differ.
+ assertEquals(update2.project(), restoredUpdate2.project());
+ assertEquals(update2.uri(), restoredUpdate2.uri());
+ assertEquals(update2.remote(), restoredUpdate2.remote());
+ assertEquals(update2.refs(), restoredUpdate2.refs());
}
@Test
@@ -447,6 +456,39 @@
update);
}
+ @SuppressWarnings("deprecation")
+ @Test
+ public void schedulingTaskFromOldFormatTasksIsSuccessful() throws Exception {
+ Gson gson =
+ new GsonBuilder()
+ .registerTypeAdapterFactory(new ReplicateRefUpdateTypeAdapterFactory())
+ .create();
+ ReplicateRefUpdate update =
+ gson.fromJson(
+ "{\"project\":\"someProject\",\"ref\":\"ref1\",\"uri\":\"git://host1/someRepo.git\",\"remote\":\"someRemote\"}",
+ ReplicateRefUpdate.class);
+
+ String oldTaskKey =
+ ObjectId.fromRaw(
+ Hashing.sha1()
+ .hashString("someProject\nref1\ngit://host1/someRepo.git\nsomeRemote", UTF_8)
+ .asBytes())
+ .name();
+ String json = gson.toJson(update) + "\n";
+ Path tmp =
+ Files.createTempFile(
+ Files.createDirectories(fileSystem.getPath("replication_site").resolve("building")),
+ oldTaskKey,
+ null);
+ Files.write(tmp, json.getBytes(UTF_8));
+
+ Task task = tasksStorage.new Task(update);
+ task.rename(tmp, task.waiting);
+
+ task.start();
+ assertIsRunning(task);
+ }
+
protected static void assertIsWaiting(Task task) {
assertTrue(task.isWaiting());
}
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 35254de..7061c79 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -43,8 +43,12 @@
protected static final URIish URISH = getUrish("http://example.com/" + PROJECT + ".git");
protected static final ReplicateRefUpdate REF_UPDATE =
ReplicateRefUpdate.create(PROJECT, Set.of(REF), URISH, REMOTE);
+ protected static final ReplicateRefUpdate STORED_REF_UPDATE =
+ ReplicateRefUpdate.create(REF_UPDATE, REF_UPDATE.sha1());
protected static final ReplicateRefUpdate REFS_UPDATE =
ReplicateRefUpdate.create(PROJECT, Set.of(REF, REF_2), URISH, REMOTE);
+ protected static final ReplicateRefUpdate STORED_REFS_UPDATE =
+ ReplicateRefUpdate.create(REFS_UPDATE, REFS_UPDATE.sha1());
protected ReplicationTasksStorage storage;
protected FileSystem fileSystem;
@@ -73,7 +77,7 @@
@Test
public void canListWaitingUpdate() throws Exception {
storage.create(REF_UPDATE);
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE);
}
@Test
@@ -91,7 +95,7 @@
assertThat(storage.start(uriUpdates)).containsExactly(REF_UPDATE.refs());
assertThatStream(storage.streamWaiting()).isEmpty();
assertFalse(storage.isWaiting(uriUpdates));
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
}
@Test
@@ -101,7 +105,7 @@
assertThat(storage.start(updates)).containsExactly(REFS_UPDATE.refs());
assertThatStream(storage.streamWaiting()).isEmpty();
assertFalse(storage.isWaiting(updates));
- assertThatStream(storage.streamRunning()).containsExactly(REFS_UPDATE);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REFS_UPDATE);
}
@Test
@@ -120,14 +124,14 @@
assertThatStream(persistedView.streamWaiting()).isEmpty();
storage.create(REF_UPDATE);
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE);
- assertThatStream(persistedView.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE);
+ assertThatStream(persistedView.streamWaiting()).containsExactly(STORED_REF_UPDATE);
storage.start(uriUpdates);
assertThatStream(storage.streamWaiting()).isEmpty();
assertThatStream(persistedView.streamWaiting()).isEmpty();
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
- assertThatStream(persistedView.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
+ assertThatStream(persistedView.streamRunning()).containsExactly(STORED_REF_UPDATE);
storage.finish(uriUpdates);
assertThatStream(storage.streamRunning()).isEmpty();
@@ -139,7 +143,7 @@
String key = storage.create(REF_UPDATE);
String secondKey = storage.create(REF_UPDATE);
assertEquals(key, secondKey);
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE);
}
@Test
@@ -171,13 +175,14 @@
storage.create(REF_UPDATE);
storage.create(updateB);
+ ReplicateRefUpdate storedUpdateB = ReplicateRefUpdate.create(updateB, updateB.sha1());
storage.start(uriUpdates);
- assertThatStream(storage.streamWaiting()).containsExactly(updateB);
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(storedUpdateB);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
storage.start(uriUpdatesB);
assertThatStream(storage.streamWaiting()).isEmpty();
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE, updateB);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE, storedUpdateB);
}
@Test
@@ -195,7 +200,8 @@
storage.start(uriUpdatesB);
storage.finish(uriUpdates);
- assertThatStream(storage.streamRunning()).containsExactly(updateB);
+ assertThatStream(storage.streamRunning())
+ .containsExactly(ReplicateRefUpdate.create(updateB, updateB.sha1()));
storage.finish(uriUpdatesB);
assertThatStream(storage.streamRunning()).isEmpty();
@@ -246,7 +252,8 @@
storage.start(uriUpdatesB);
storage.finish(uriUpdatesA);
- assertThatStream(storage.streamRunning()).containsExactly(refUpdateB);
+ assertThatStream(storage.streamRunning())
+ .containsExactly(ReplicateRefUpdate.create(refUpdateB, refUpdateB.sha1()));
storage.finish(uriUpdatesB);
assertThatStream(storage.streamRunning()).isEmpty();
@@ -258,7 +265,7 @@
storage.start(uriUpdates);
storage.reset(uriUpdates);
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE);
assertThatStream(storage.streamRunning()).isEmpty();
}
@@ -269,7 +276,7 @@
storage.reset(uriUpdates);
storage.start(uriUpdates);
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
assertThatStream(storage.streamWaiting()).isEmpty();
assertFalse(storage.isWaiting(uriUpdates));
@@ -289,7 +296,7 @@
storage.start(uriUpdates);
storage.recoverAll();
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE);
assertThatStream(storage.streamRunning()).isEmpty();
assertTrue(storage.isWaiting(uriUpdates));
}
@@ -301,7 +308,7 @@
storage.recoverAll();
storage.start(uriUpdates);
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
assertThatStream(storage.streamWaiting()).isEmpty();
assertFalse(storage.isWaiting(uriUpdates));
@@ -324,7 +331,8 @@
storage.start(uriUpdatesB);
storage.recoverAll();
- assertThatStream(storage.streamWaiting()).containsExactly(REF_UPDATE, updateB);
+ assertThatStream(storage.streamWaiting())
+ .containsExactly(STORED_REF_UPDATE, ReplicateRefUpdate.create(updateB, updateB.sha1()));
}
@Test
@@ -342,12 +350,13 @@
storage.start(uriUpdatesB);
storage.recoverAll();
+ ReplicateRefUpdate storedUpdateB = ReplicateRefUpdate.create(updateB, updateB.sha1());
storage.start(uriUpdates);
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE);
- assertThatStream(storage.streamWaiting()).containsExactly(updateB);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE);
+ assertThatStream(storage.streamWaiting()).containsExactly(storedUpdateB);
storage.start(uriUpdatesB);
- assertThatStream(storage.streamRunning()).containsExactly(REF_UPDATE, updateB);
+ assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE, storedUpdateB);
assertThatStream(storage.streamWaiting()).isEmpty();
storage.finish(uriUpdates);
