Revert "Revert "Keep reply with most severe error when merging error replies""

This reverts commit 9263966a9c697f5acff53a595493e27a2f05e683.

6 files changed, 45 insertions, 42 deletions

diff --git a/document/src/main/java/com/yahoo/document/BucketId.java b/document/src/main/java/com/yahoo/document/BucketId.java
index 36ec53c51d3..f6d3dc34af1 100755
--- a/document/src/main/java/com/yahoo/document/BucketId.java
+++ b/document/src/main/java/com/yahoo/document/BucketId.java
@@ -74,9 +74,9 @@ public class BucketId implements Comparable<BucketId> {
 
     public int compareTo(BucketId other) {
         if (id >>> 32 == other.id >>> 32) {
-            if ((id & 0xFFFFFFFFl) > (other.id & 0xFFFFFFFFl)) {
+            if ((id & 0xFFFFFFFFL) > (other.id & 0xFFFFFFFFL)) {
                 return 1;
-            } else if ((id & 0xFFFFFFFFl) < (other.id & 0xFFFFFFFFl)) {
+            } else if ((id & 0xFFFFFFFFL) < (other.id & 0xFFFFFFFFL)) {
                 return -1;
             }
             return 0;
@@ -97,8 +97,8 @@ public class BucketId implements Comparable<BucketId> {
 
     public long getId() {
         int notUsed = 64 - getUsedBits();
-        long usedMask = (0xFFFFFFFFFFFFFFFFl << notUsed) >>> notUsed;
-        long countMask = (0xFFFFFFFFFFFFFFFFl >>> (64 - COUNT_BITS)) << (64 - COUNT_BITS);
+        long usedMask  = (0xFFFFFFFFFFFFFFFFL << notUsed) >>> notUsed;
+        long countMask = (0xFFFFFFFFFFFFFFFFL >>> (64 - COUNT_BITS)) << (64 - COUNT_BITS);
         return id & (usedMask | countMask);
     }
 
diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentProtocol.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentProtocol.java
index f5b4920fa3f..2680ed011af 100755
--- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentProtocol.java
+++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/DocumentProtocol.java
@@ -429,7 +429,7 @@ public class DocumentProtocol implements Protocol {
      * @param ctx the context whose children to merge
      */
     public static void merge(RoutingContext ctx) {
-        merge(ctx, new HashSet<Integer>(0));
+        merge(ctx, new HashSet<>(0));
     }
 
     /**
@@ -475,7 +475,7 @@ public class DocumentProtocol implements Protocol {
      * @return the merged Reply
      */
     public static Reply merge(List<Reply> replies) {
-        return merge(replies, new HashSet<Integer>(0)).second;
+        return merge(replies, new HashSet<>(0)).second;
     }
 
     /**
diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/ReplyMerger.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/ReplyMerger.java
index a8ec53c5c97..630a8588495 100644
--- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/ReplyMerger.java
+++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/ReplyMerger.java
@@ -3,6 +3,7 @@ package com.yahoo.documentapi.messagebus.protocol;
 
 import com.yahoo.collections.Tuple2;
 import com.yahoo.messagebus.EmptyReply;
+import com.yahoo.messagebus.Error;
 import com.yahoo.messagebus.Reply;
 
 /**
@@ -57,15 +58,22 @@ final class ReplyMerger {
             return;
         }
         if (error == null) {
-            error = new EmptyReply();
-            r.swapState(error);
-            return;
+            error = r;
+        }
+        else if (mostSevereErrorCode(r) > mostSevereErrorCode(error)) {
+            error.getErrors().forEach(r::addError);
+            error = r;
         }
-        for (int j = 0; j < r.getNumErrors(); ++j) {
-            error.addError(r.getError(j));
+        else {
+            r.getErrors().forEach(error::addError);
         }
     }
 
+    private static int mostSevereErrorCode(Reply reply) {
+        return reply.getErrors().mapToInt(Error::getCode).max()
+                .orElseThrow(() -> new IllegalArgumentException(reply + " has no errors"));
+    }
+
     private boolean handleReplyWithOnlyIgnoredErrors(Reply r) {
         if (DocumentProtocol.hasOnlyErrorsOfType(r, DocumentProtocol.ERROR_MESSAGE_IGNORED)) {
             if (ignore == null) {
@@ -96,7 +104,7 @@ final class ReplyMerger {
     }
 
     private Tuple2<Integer, Reply> createEmptyReplyResult() {
-        return new Tuple2<>(null, (Reply)new EmptyReply());
+        return new Tuple2<>(null, new EmptyReply());
     }
 
     public Tuple2<Integer, Reply> mergedReply() {
diff --git a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/ReplyMergerTestCase.java b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/ReplyMergerTestCase.java
index 157b4a6585b..11d74800a79 100644
--- a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/ReplyMergerTestCase.java
+++ b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/ReplyMergerTestCase.java
@@ -38,7 +38,7 @@ public class ReplyMergerTestCase {
     }
 
     @Test
-    public void mergingSingleReplyWithOneErrorReturnsEmptyReplyWithError() {
+    public void mergingSingleReplyWithOneErrorReturnsSameReplyWithError() {
         Reply r1 = new EmptyReply();
         Error error = new Error(1234, "oh no!");
         r1.addError(error);
@@ -46,12 +46,12 @@ public class ReplyMergerTestCase {
         Tuple2<Integer, Reply> ret = merger.mergedReply();
 
         assertNull(ret.first);
-        assertNotSame(r1, ret.second);
+        assertSame(r1, ret.second);
         assertThatErrorsMatch(new Error[] { error }, ret);
     }
 
     @Test
-    public void mergingSingleReplyWithMultipleErrorsReturnsEmptyReplyWithAllErrors() {
+    public void mergingSingleReplyWithMultipleErrorsReturnsSameReplyWithAllErrors() {
         Reply r1 = new EmptyReply();
         Error errors[] = new Error[] {
                 new Error(1234, "oh no!"), new Error(4567, "oh dear!"),
@@ -62,12 +62,12 @@ public class ReplyMergerTestCase {
         Tuple2<Integer, Reply> ret = merger.mergedReply();
 
         assertNull(ret.first);
-        assertNotSame(r1, ret.second);
+        assertSame(r1, ret.second);
         assertThatErrorsMatch(errors, ret);
     }
 
     @Test
-    public void mergingMultipleRepliesWithMultipleErrorsReturnsEmptyReplyWithAllErrors() {
+    public void mergingMultipleRepliesWithMultipleErrorsReturnsMostSevereReplyWithAllErrors() {
         Reply r1 = new EmptyReply();
         Reply r2 = new EmptyReply();
         Error errors[] = new Error[] {
@@ -81,7 +81,7 @@ public class ReplyMergerTestCase {
         Tuple2<Integer, Reply> ret = merger.mergedReply();
 
         assertNull(ret.first);
-        assertNotSame(r1, ret.second);
+        assertSame(r1, ret.second);
         assertNotSame(r2, ret.second);
         assertThatErrorsMatch(errors, ret);
     }
@@ -143,7 +143,7 @@ public class ReplyMergerTestCase {
         merger.merge(1, r2);
         Tuple2<Integer, Reply> ret = merger.mergedReply();
         assertNull(ret.first);
-        assertNotSame(r1, ret.second);
+        assertSame(r1, ret.second);
         assertNotSame(r2, ret.second);
         // All errors from replies with errors are included, not those that
         // are fully ignored.
@@ -182,7 +182,7 @@ public class ReplyMergerTestCase {
     }
 
     @Test
-    // TODO: This seems wrong, and is probably a consequence of TAS being added later than this logic was written.
+    // TODO jonmv: This seems wrong, and is probably a consequence of TAS being implemented after reply merging.
     public void returnErrorDocumentReplyWhereDocWasFoundWhichIsProbablyWrong() {
         Error e1 = new Error(DocumentProtocol.ERROR_TEST_AND_SET_CONDITION_FAILED, "fail");
         UpdateDocumentReply r1 = new UpdateDocumentReply();
@@ -197,7 +197,7 @@ public class ReplyMergerTestCase {
         merger.merge(2, r3);
         Tuple2<Integer, Reply> ret = merger.mergedReply();
         assertNull(ret.first);
-        assertNotSame(r1, ret.second);
+        assertSame(r1, ret.second);
         assertThatErrorsMatch(new Error[] { e1 }, ret);
     }
 
diff --git a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/test/PolicyTestFrame.java b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/test/PolicyTestFrame.java
index 89d5db62899..92e94256411 100755
--- a/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/test/PolicyTestFrame.java
+++ b/documentapi/src/test/java/com/yahoo/documentapi/messagebus/protocol/test/PolicyTestFrame.java
@@ -155,16 +155,6 @@ public class PolicyTestFrame {
     }
 
     /**
-     * This is a convenience method for invoking {@link #assertMerge(Map,List,List)} with no expected value.
-     *
-     * @param replies        The errors to set in the leaf node replies.
-     * @param expectedErrors The list of expected errors in the merged reply.
-     */
-    public void assertMergeError(Map<String, Integer> replies, List<Integer> expectedErrors) {
-        assertMerge(replies, expectedErrors, null);
-    }
-
-    /**
      * This is a convenience method for invoking {@link this#assertMerge(Map,List,List)} with no expected errors.
      *
      * @param replies       The errors to set in the leaf node replies.
@@ -233,10 +223,10 @@ public class PolicyTestFrame {
 
         Map<String, Integer> replies = new HashMap<>();
         replies.put(recipient, ErrorCode.NONE);
-        assertMergeOk(replies, Arrays.asList(recipient));
+        assertMergeOk(replies, List.of(recipient));
 
         replies.put(recipient, ErrorCode.TRANSIENT_ERROR);
-        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR));
+        assertMerge(replies, List.of(ErrorCode.TRANSIENT_ERROR), List.of(recipient));
     }
 
     /**
@@ -252,28 +242,33 @@ public class PolicyTestFrame {
         Map<String, Integer> replies = new HashMap<>();
         replies.put(recipientOne, ErrorCode.NONE);
         replies.put(recipientTwo, ErrorCode.NONE);
-        assertMergeOk(replies, Arrays.asList(recipientOne, recipientTwo));
+        assertMergeOk(replies, List.of(recipientOne, recipientTwo));
 
         replies.put(recipientOne, ErrorCode.TRANSIENT_ERROR);
         replies.put(recipientTwo, ErrorCode.NONE);
-        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR));
+        assertMerge(replies, List.of(ErrorCode.TRANSIENT_ERROR), List.of(recipientOne));
+
+        replies.put(recipientOne, ErrorCode.TRANSIENT_ERROR);
+        replies.put(recipientTwo, ErrorCode.FATAL_ERROR);
+        assertMerge(replies, List.of(ErrorCode.TRANSIENT_ERROR, ErrorCode.FATAL_ERROR), List.of(recipientTwo));
 
         replies.put(recipientOne, ErrorCode.TRANSIENT_ERROR);
         replies.put(recipientTwo, ErrorCode.TRANSIENT_ERROR);
-        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR, ErrorCode.TRANSIENT_ERROR));
+        assertMerge(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR, ErrorCode.TRANSIENT_ERROR), List.of(recipientOne, recipientTwo));
 
         replies.put(recipientOne, ErrorCode.NONE);
         replies.put(recipientTwo, DocumentProtocol.ERROR_MESSAGE_IGNORED);
-        assertMergeOk(replies, Arrays.asList(recipientOne));
+        assertMergeOk(replies, List.of(recipientOne));
 
         replies.put(recipientOne, DocumentProtocol.ERROR_MESSAGE_IGNORED);
         replies.put(recipientTwo, ErrorCode.NONE);
-        assertMergeOk(replies, Arrays.asList(recipientTwo));
+        assertMergeOk(replies, List.of(recipientTwo));
 
         replies.put(recipientOne, DocumentProtocol.ERROR_MESSAGE_IGNORED);
         replies.put(recipientTwo, DocumentProtocol.ERROR_MESSAGE_IGNORED);
-        assertMergeError(replies, Arrays.asList(DocumentProtocol.ERROR_MESSAGE_IGNORED,
-                                                DocumentProtocol.ERROR_MESSAGE_IGNORED));
+        assertMerge(replies, List.of(DocumentProtocol.ERROR_MESSAGE_IGNORED,
+                                     DocumentProtocol.ERROR_MESSAGE_IGNORED),
+                    null); // Only ignored errors specifically causes an EmptyReply.
     }
 
     /**
diff --git a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingTable.java b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingTable.java
index 82f409dbb44..e3410cdba7d 100644
--- a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingTable.java
+++ b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingTable.java
@@ -13,8 +13,8 @@ import java.util.Map;
  */
 public class RoutingTable {
 
-    private final Map<String, HopBlueprint> hops = new LinkedHashMap<String, HopBlueprint>();
-    private final Map<String, Route> routes = new LinkedHashMap<String, Route>();
+    private final Map<String, HopBlueprint> hops = new LinkedHashMap<>();
+    private final Map<String, Route> routes = new LinkedHashMap<>();
 
     /**
      * Creates a new routing table based on a given specification. This also verifies the integrity of the table.