Merge pull request #10984 from vespa-engine/mpolden/cleanup

Minor RotationRepository cleanup

2 files changed, 19 insertions, 18 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
index 9f6bbcd2a5a..1d475c985a4 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
@@ -6,11 +6,9 @@ import com.yahoo.config.application.api.Endpoint;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
-import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
+import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
@@ -58,21 +56,14 @@ public class RotationRepository {
         return new RotationLock(curator.lockRotations());
     }
 
-    /** Get rotation for given application */
-    public Optional<Rotation> getRotation(Instance instance) {
-        return instance.rotations().stream()
-                       .map(AssignedRotation::rotationId)
-                       .map(allRotations::get)
-                       .findFirst();
-    }
-
-    /** Get rotation for the given rotationId */
+    /** Get rotation by given rotationId */
     public Optional<Rotation> getRotation(RotationId rotationId) {
         return Optional.of(allRotations.get(rotationId));
     }
 
     /**
-     * Returns a rotation for the given application
+     * Returns a single rotation for the given application. This is only used when a rotation is assigned through the
+     * use of a global service ID.
      *
      * If a rotation is already assigned to the application, that rotation will be returned.
      * If no rotation is assigned, return an available rotation. The caller is responsible for assigning the rotation.
@@ -81,7 +72,7 @@ public class RotationRepository {
      * @param instance the instance requesting a rotation
      * @param lock lock which must be acquired by the caller
      */
-    public Rotation getOrAssignRotation(DeploymentSpec deploymentSpec, Instance instance, RotationLock lock) {
+    private Rotation getOrAssignRotation(DeploymentSpec deploymentSpec, Instance instance, RotationLock lock) {
         if ( ! instance.rotations().isEmpty()) {
             return allRotations.get(instance.rotations().get(0).rotationId());
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
index 25ae7c88e84..b298cfcc889 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
@@ -73,8 +73,10 @@ public class RotationRepositoryTest {
         assertEquals(URI.create("https://app1--tenant1.global.vespa.oath.cloud:4443/"),
                      instance.endpointsIn(SystemName.main).main().get().url());
         try (RotationLock lock = repository.lock()) {
-            Rotation rotation = repository.getOrAssignRotation(application.deploymentSpec(), tester.applications().requireInstance(instance.id()), lock);
-            assertEquals(expected, rotation);
+            List<AssignedRotation> rotations = repository.getOrAssignRotations(application.deploymentSpec(),
+                                                                              tester.applications().requireInstance(instance.id()),
+                                                                              lock);
+            assertSingleRotation(expected, rotations, repository);
         }
 
         // Deploying once more assigns same rotation
@@ -98,9 +100,9 @@ public class RotationRepositoryTest {
         Instance instance2 = tester.defaultInstance(application2.id());
 
         try (RotationLock lock = repository.lock()) {
-            Rotation rotation = repository.getOrAssignRotation(application2.deploymentSpec(), instance2, lock);
+            List<AssignedRotation> rotations = repository.getOrAssignRotations(application2.deploymentSpec(), instance2, lock);
             Rotation assignedRotation = new Rotation(new RotationId("foo-1"), "foo-1.com");
-            assertEquals(assignedRotation, rotation);
+            assertSingleRotation(assignedRotation, rotations, repository);
         }
     }
 
@@ -156,6 +158,14 @@ public class RotationRepositoryTest {
                 .endpointsIn(SystemName.cd).main().get().url().toString());
     }
 
+    private void assertSingleRotation(Rotation expected, List<AssignedRotation> assignedRotations, RotationRepository repository) {
+        assertEquals(1, assignedRotations.size());
+        var rotationId = assignedRotations.get(0).rotationId();
+        var rotation = repository.getRotation(rotationId);
+        assertTrue(rotationId + " exists", rotation.isPresent());
+        assertEquals(expected, rotation.get());
+    }
+
     private static List<RotationId> rotationIds(List<AssignedRotation> assignedRotations) {
         return assignedRotations.stream().map(AssignedRotation::rotationId).collect(Collectors.toUnmodifiableList());
     }