From 776dc24d57f0d9f49412eeb33e4c912c7fe561c9 Mon Sep 17 00:00:00 2001 From: Valerij Fredriksen Date: Fri, 6 May 2022 10:46:58 +0200 Subject: Code review fixes --- .../api/integration/artifact/Artifact.java | 29 +++++++--------------- .../api/integration/artifact/ArtifactRegistry.java | 2 +- .../controller/maintenance/ArtifactExpirer.java | 6 ++--- .../integration/ArtifactRegistryMock.java | 10 ++++---- .../maintenance/ArtifactExpirerTest.java | 4 +-- 5 files changed, 20 insertions(+), 31 deletions(-) diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/Artifact.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/Artifact.java index e0a4e0c0ae8..7ca372f6cd0 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/Artifact.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/Artifact.java @@ -5,7 +5,6 @@ import com.yahoo.component.Version; import java.time.Instant; import java.util.Objects; -import java.util.Optional; /** * A registry artifact (e.g. container image or RPM) @@ -15,25 +14,16 @@ import java.util.Optional; public class Artifact { private final String id; - private final Optional registry; - private final Optional repository; - private final Optional tag; + private final String registry; + private final String repository; + private final String tag; private final Instant createdAt; private final Version version; public Artifact(String id, String registry, String repository, String tag, Instant createdAt, Version version) { this.id = Objects.requireNonNull(id); - this.registry = Optional.of(registry); - this.repository = Optional.of(repository); - this.tag = Optional.of(tag); - this.createdAt = Objects.requireNonNull(createdAt); - this.version = Objects.requireNonNull(version); - } - - public Artifact(String id, Instant createdAt, Optional tag, Version version) { - this.id = Objects.requireNonNull(id); - this.registry = Optional.empty(); - this.repository = Optional.empty(); + this.registry = Objects.requireNonNull(registry); + this.repository = Objects.requireNonNull(repository); this.tag = Objects.requireNonNull(tag); this.createdAt = Objects.requireNonNull(createdAt); this.version = Objects.requireNonNull(version); @@ -45,17 +35,17 @@ public class Artifact { } /** The registry holding this artifact */ - public Optional registry() { + public String registry() { return registry; } /** Repository of this artifact */ - public Optional repository() { + public String repository() { return repository; } /** Tag of this artifact */ - public Optional tag() { + public String tag() { return tag; } @@ -89,7 +79,6 @@ public class Artifact { @Override public String toString() { - String name = repository.isPresent() ? registry.get() + "/" + repository.get() : id; - return "artifact " + name + " [version=" + version.toFullString() + ",createdAt=" + createdAt + tag.map(t -> ",tag=" + t).orElse("") + "]"; + return "artifact " + registry + "/" + repository + " [version=" + version.toFullString() + ",createdAt=" + createdAt + ",tag=" + tag + "]"; } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/ArtifactRegistry.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/ArtifactRegistry.java index 8d3c19083c9..6ab8409ad11 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/ArtifactRegistry.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/artifact/ArtifactRegistry.java @@ -12,7 +12,7 @@ import java.util.List; public interface ArtifactRegistry { /** Delete all given artifacts */ - void deleteAll(List images); + void deleteAll(List artifacts); /** Returns a list of all artifacts in this system */ List list(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirer.java index 47eeb291dd7..81fbb85e3d7 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirer.java @@ -37,18 +37,18 @@ public class ArtifactExpirer extends ControllerMaintainer { @Override protected double maintain() { + VersionStatus versionStatus = controller().readVersionStatus(); return controller().clouds().stream() .flatMapToDouble(cloud -> controller().serviceRegistry().artifactRegistry(cloud).stream() - .mapToDouble(artifactRegistry -> maintain(cloud, artifactRegistry))) + .mapToDouble(artifactRegistry -> maintain(versionStatus, cloud, artifactRegistry))) .average() .orElse(1); } - private double maintain(CloudName cloudName, ArtifactRegistry artifactRegistry) { + private double maintain(VersionStatus versionStatus, CloudName cloudName, ArtifactRegistry artifactRegistry) { try { Instant now = controller().clock().instant(); - VersionStatus versionStatus = controller().readVersionStatus(); List artifactsToExpire = artifactRegistry.list().stream() .filter(artifact -> isExpired(artifact, now, versionStatus)) .collect(Collectors.toList()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ArtifactRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ArtifactRegistryMock.java index 1d3974fe7a7..ef0445e06c6 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ArtifactRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ArtifactRegistryMock.java @@ -15,9 +15,9 @@ import java.util.stream.Collectors; */ public class ArtifactRegistryMock implements ArtifactRegistry { - private static final Comparator comparator = Comparator.comparing((Artifact artifact) -> artifact.registry().orElse("")) - .thenComparing(artifact -> artifact.repository().orElse("")) - .thenComparing(Artifact::version); + private static final Comparator comparator = Comparator.comparing(Artifact::registry) + .thenComparing(Artifact::repository) + .thenComparing(Artifact::version); private final Map images = new HashMap<>(); @@ -27,8 +27,8 @@ public class ArtifactRegistryMock implements ArtifactRegistry { } @Override - public void deleteAll(List images) { - images.forEach(image -> this.images.remove(image.id())); + public void deleteAll(List artifacts) { + artifacts.forEach(image -> this.images.remove(image.id())); } @Override diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java index 9a19d4d0185..7703266c9ba 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArtifactExpirerTest.java @@ -26,8 +26,8 @@ public class ArtifactExpirerTest { ArtifactRegistryMock registry = tester.controllerTester().serviceRegistry().artifactRegistry(CloudName.defaultName()).orElseThrow(); Instant instant = tester.clock().instant(); - Artifact image0 = new Artifact("image0", "registry.example.com", "vespa/vespa", "7.4", instant, Version.fromString("7.1")); - Artifact image1 = new Artifact("image1", "registry.example.com", "vespa/vespa", "7.4-amd64", instant, Version.fromString("7.2")); + Artifact image0 = new Artifact("image0", "registry.example.com", "vespa/vespa", "7.1", instant, Version.fromString("7.1")); + Artifact image1 = new Artifact("image1", "registry.example.com", "vespa/vespa", "7.2-amd64", instant, Version.fromString("7.2")); Artifact image2 = new Artifact("image2", "registry.example.com", "vespa/vespa", "7.4-amd64", instant, Version.fromString("7.4")); registry.add(image0) .add(image1) -- cgit v1.2.3