* [PATCH v1] TasksStorage: Remove delete() only used by tests
@ 2020-08-05 7:16 Adithya Chakilam
0 siblings, 0 replies; only message in thread
From: Adithya Chakilam @ 2020-08-05 7:16 UTC (permalink / raw)
To: git; +Cc: Nasser Grainawi
From: Nasser Grainawi <nasser@codeaurora.org>
In general we shouldn't have methods that provide behavior only for
tests. Callers of delete() in pre-3.1 code should move to start() and
finish() in 3.1. To facilitate that, update 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.
Change-Id: Id82d2ec3125075832134c8dbe56b342e5a874bbc
---
.../gerrit/plugins/replication/PushOne.java | 8 ++-
.../replication/ReplicationTasksStorage.java | 52 +++++++-------
.../plugins/replication/ReplicationIT.java | 15 +++-
.../ReplicationTasksStorageTest.java | 68 ++++++++++++-------
.../plugins/replication/TestUriUpdates.java | 52 ++++++++++++++
5 files changed, 139 insertions(+), 56 deletions(-)
create mode 100644 src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
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..3124940 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 @@ import org.slf4j.MDC;
* <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, ReplicationTasksStorage.UriUpdates {
private final ReplicationStateListener stateLog;
static final String ALL_REFS = "..all..";
static final String ID_MDC_KEY = "pushOneId";
@@ -230,7 +230,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {
return canceled || canceledWhileRunning.get();
}
- URIish getURI() {
+ @Override
+ public URIish getURI() {
return uri;
}
@@ -244,7 +245,8 @@ class PushOne implements ProjectRunnable, CanceledWhileRunning {
}
}
- 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..8a5aba2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -19,6 +19,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.Hashing;
+import com.google.gerrit.entities.Project;
import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.ProvisionException;
@@ -32,6 +33,7 @@ import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.List;
+import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.transport.URIish;
@@ -68,8 +70,12 @@ public class ReplicationTasksStorage {
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) {
@@ -85,6 +91,16 @@ public class ReplicationTasksStorage {
}
}
+ public interface UriUpdates {
+ Project.NameKey getProjectNameKey();
+
+ URIish getURI();
+
+ String getRemoteName();
+
+ Set<String> getRefs();
+ }
+
private static final Gson GSON = new Gson();
private final Path refUpdates;
@@ -114,20 +130,15 @@ public class ReplicationTasksStorage {
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 +148,9 @@ public class ReplicationTasksStorage {
}
}
- 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 +272,6 @@ public class ReplicationTasksStorage {
}
}
- 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/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
index 31cd75d..bb0f283 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationIT.java
@@ -36,7 +36,9 @@ import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
import com.google.inject.Key;
import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
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 +451,7 @@ public class ReplicationIT extends LightweightPluginDaemonTest {
}
@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 +465,16 @@ public class ReplicationIT extends LightweightPluginDaemonTest {
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..c102e14 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;
import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
+import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
import java.net.URISyntaxException;
import java.nio.file.FileSystem;
import java.nio.file.Path;
@@ -42,12 +43,14 @@ public class ReplicationTasksStorageTest {
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 +70,10 @@ public class ReplicationTasksStorageTest {
}
@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 +88,8 @@ public class ReplicationTasksStorageTest {
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 +118,23 @@ public class ReplicationTasksStorageTest {
}
@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 +166,55 @@ public class ReplicationTasksStorageTest {
}
@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);
-
- storage.delete(refA);
- assertContainsExactly(storage, refB);
-
- storage.delete(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.finish(uriUpdatesA);
+ assertContainsExactly(storage, refUpdateB);
+
+ 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..b9e9701
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/TestUriUpdates.java
@@ -0,0 +1,52 @@
+// 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 com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.UriUpdates;
+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();
+}
--
2.24.3 (Apple Git-128)
^ permalink raw reply related [flat|nested] only message in thread
only message in thread, other threads:[~2020-08-05 7:16 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05 7:16 [PATCH v1] TasksStorage: Remove delete() only used by tests Adithya Chakilam
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.