Minor simplification and javadoc improvements

4 files changed, 55 insertions, 46 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueue.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueue.java
index ab22b551b2b..3996ef671aa 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueue.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueue.java
@@ -14,8 +14,6 @@ import java.util.function.UnaryOperator;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
-import static java.util.function.Predicate.not;
-
 /**
  * A queue of outstanding {@link NameServiceRequest}s. Requests in this have not yet been dispatched to a
  * {@link NameService} and are thus not visible in DNS.
@@ -69,13 +67,22 @@ public record NameServiceQueue(List<NameServiceRequest> requests) {
         return new NameServiceQueue(copy);
     }
 
+    /** Returns a copy of this without the requests present in other. Duplicates are not removed */
+    public NameServiceQueue without(NameServiceQueue other) {
+        List<NameServiceRequest> toRemove = new ArrayList<>(other.requests);
+        return new NameServiceQueue(requests.stream()
+                                            .filter(request -> !toRemove.remove(request))
+                                            .toList());
+    }
+
     /** Returns a copy of this with given request added */
     public NameServiceQueue with(NameServiceRequest request) {
         return with(request, Priority.normal);
     }
 
     /**
-     * Dispatch n requests from the head of this to given name service.
+     * Dispatch n requests from the head of this to given name service. Requests may be re-ordered if errors are
+     * encountered, but are always dispatched in order within an application.
      *
      * @return A copy of this, without the successfully dispatched requests.
      */
@@ -88,13 +95,12 @@ public record NameServiceQueue(List<NameServiceRequest> requests) {
             NameServiceRequest request = pending.poll();
             try {
                 request.dispatchTo(nameService);
-            }
-            catch (Exception e) {
+            } catch (Exception e) {
                 boolean dropFailingRequest = pending.size() > QUEUE_CAPACITY;
                 log.log(Level.WARNING, "Failed to execute " + request + ": " + Exceptions.toMessageString(e) +
                                        ", request will " + (dropFailingRequest ? "be dropped, as queue is over capacity"
                                                                                : "be moved backwards, and retried"));
-                if (dropFailingRequest) continue;
+                if (dropFailingRequest) continue; // Drop this request and keep dispatching others
 
                 // Move all requests with the same owner backwards as far as we can, i.e., to the back, or to the first owner-less request.
                 Optional<TenantAndApplicationId> owner = request.owner();
@@ -106,8 +112,7 @@ public record NameServiceQueue(List<NameServiceRequest> requests) {
                         break;  // Can't modify anything past this, as owner-less requests must come in order with all others.
                     }
                     (request.owner().equals(owner) ? owned : others).offer(request);
-                }
-                while ((request = pending.poll()) != null);
+                } while ((request = pending.poll()) != null);
                 pending.addAll(0, owned);   // Append owned requests before those we can't modify (or none), and
                 pending.addAll(0, others);  // then append requests owned by others before that again.
             }
@@ -137,49 +142,48 @@ public record NameServiceQueue(List<NameServiceRequest> requests) {
         if (n < 0) throw new IllegalArgumentException("n must be >= 0, got " + n);
     }
 
-    /** Priority of a request added to this */
-    public enum Priority {
-
-        /** Default priority. Request will be delivered in FIFO order */
-        normal,
-
-        /** Request is queued first. Useful for code that needs to act on effects of a request */
-        high
-
-    }
-
-    /** Returns the queue of records present in this, but not in {@code remaining}. */
-    public NameServiceQueue minus(NameServiceQueue remaining) {
-        return new NameServiceQueue(requests.stream()
-                                            .filter(not(new LinkedList<>(remaining.requests())::remove)) // Count duplicates.
-                                            .toList());
-    }
-
-    /** Replaces the sublist {@code initial} contained in this with {@code remaining}, or best effort amendment when not contained. */
-    public NameServiceQueue replace(NameServiceQueue initial, NameServiceQueue remaining) {
-        int sublistIndex = indexOf(requests, initial.requests);
+    /**
+     * Replaces the requests in {@code oldQueue} contained in this with requests in {@code newQueue}, or best effort
+     * amendment when not contained.
+     */
+    public NameServiceQueue replace(NameServiceQueue oldQueue, NameServiceQueue newQueue) {
+        int sublistIndex = indexOf(oldQueue.requests, requests);
         if (sublistIndex >= 0) {
-            LinkedList<NameServiceRequest> updated = new LinkedList<>();
+            List<NameServiceRequest> updated = new ArrayList<>();
             updated.addAll(requests.subList(0, sublistIndex));
-            updated.addAll(remaining.requests);
-            updated.addAll(requests.subList(sublistIndex + initial.requests.size(), requests.size()));
+            updated.addAll(newQueue.requests);
+            updated.addAll(requests.subList(sublistIndex + oldQueue.requests.size(), requests.size()));
             return new NameServiceQueue(updated);
-        }
-        else {
+        } else {
             log.log(Level.WARNING, "Name service queue has changed unexpectedly; expected requests: " +
-                                   initial.requests + " to be present, but that was not found in: " + requests);
+                                   oldQueue.requests + " to be present, but that was not found in: " + requests);
             // Do a best-effort amendment, where requests removed from initial to remaining, are removed, from the front, from this.
-            return minus(initial.minus(remaining));
+            return without(oldQueue.without(newQueue));
         }
     }
 
-    /** Lowest index {@code i} in {@code list} s.t. {@code list.sublist(i, i + sub.size()).equals(sub)}. Naïve implementation. */
-    static <T> int indexOf(List<T> list, List<T> sub) {
-        for (int i = 0; i + sub.size() <= list.size(); i++)
-            if (list.subList(i, i + sub.size()).equals(sub))
+    /**
+     * Find the starting index of subList in list. I.e. the lowest index {@code i} in {@code list} so that
+     * {@code list.subList(i, i + subList.size()).equals(subList)}. Naïve implementation.
+     */
+    private static <T> int indexOf(List<T> subList, List<T> list) {
+        for (int i = 0; i + subList.size() <= list.size(); i++) {
+            if (list.subList(i, i + subList.size()).equals(subList)) {
                 return i;
-
+            }
+        }
         return -1;
     }
 
+    /** Priority of a request added to this */
+    public enum Priority {
+
+        /** Default priority. Request will be delivered in FIFO order */
+        normal,
+
+        /** Request is queued first. Useful for code that needs to act on effects of a request */
+        high
+
+    }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceRequest.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceRequest.java
index 3a35c92d4e7..c43130ce4e9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceRequest.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/dns/NameServiceRequest.java
@@ -16,6 +16,7 @@ public interface NameServiceRequest {
 
     Optional<RecordName> name();
 
+    /** The application owning this request */
     Optional<TenantAndApplicationId> owner();
 
     /** Send this to given name service */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/NameServiceDispatcher.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/NameServiceDispatcher.java
index e8cb8e30cd2..e4841618852 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/NameServiceDispatcher.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/NameServiceDispatcher.java
@@ -8,7 +8,7 @@ import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
 import java.time.Clock;
 import java.time.Duration;
-import java.util.ArrayList;
+import java.time.Instant;
 import java.util.logging.Level;
 
 /**
@@ -39,15 +39,15 @@ public class NameServiceDispatcher extends ControllerMaintainer {
         // Dispatch 1 request per second on average. Note that this is not entirely accurate because a NameService
         // implementation may need to perform multiple API-specific requests to execute a single NameServiceRequest
         int requestCount = trueIntervalInSeconds();
-        NameServiceQueue initial;
+        final NameServiceQueue initial;
         try (var lock = db.lockNameServiceQueue()) {
             initial = db.readNameServiceQueue();
         }
         if (initial.requests().isEmpty() || requestCount == 0) return 1.0;
 
-        var instant = clock.instant();
-        var remaining = initial.dispatchTo(nameService, requestCount);
-        var dispatched = initial.minus(remaining);
+        Instant instant = clock.instant();
+        NameServiceQueue remaining = initial.dispatchTo(nameService, requestCount);
+        NameServiceQueue dispatched = initial.without(remaining);
 
         if (!dispatched.requests().isEmpty()) {
             Level logLevel = controller().system().isCd() ? Level.INFO : Level.FINE;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java
index 6d555e6d455..939e3252750 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/dns/NameServiceQueueTest.java
@@ -102,6 +102,10 @@ public class NameServiceQueueTest {
         assertSame(queue, queue.first(3));
         assertSame(queue, queue.first(10));
         assertTrue(queue.first(0).requests().isEmpty());
+
+        // Remove some requests
+        queue = new NameServiceQueue(List.of(req1, req2, req2, req3)).without(new NameServiceQueue(List.of(req1, req2)));
+        assertEquals(List.of(req2, req3), queue.requests());
     }
 
     @Test