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

2 files changed, 28 insertions, 23 deletions

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 11d74800a79..157b4a6585b 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 mergingSingleReplyWithOneErrorReturnsSameReplyWithError() {
+    public void mergingSingleReplyWithOneErrorReturnsEmptyReplyWithError() {
         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);
-        assertSame(r1, ret.second);
+        assertNotSame(r1, ret.second);
         assertThatErrorsMatch(new Error[] { error }, ret);
     }
 
     @Test
-    public void mergingSingleReplyWithMultipleErrorsReturnsSameReplyWithAllErrors() {
+    public void mergingSingleReplyWithMultipleErrorsReturnsEmptyReplyWithAllErrors() {
         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);
-        assertSame(r1, ret.second);
+        assertNotSame(r1, ret.second);
         assertThatErrorsMatch(errors, ret);
     }
 
     @Test
-    public void mergingMultipleRepliesWithMultipleErrorsReturnsMostSevereReplyWithAllErrors() {
+    public void mergingMultipleRepliesWithMultipleErrorsReturnsEmptyReplyWithAllErrors() {
         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);
-        assertSame(r1, ret.second);
+        assertNotSame(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);
-        assertSame(r1, ret.second);
+        assertNotSame(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 jonmv: This seems wrong, and is probably a consequence of TAS being implemented after reply merging.
+    // TODO: This seems wrong, and is probably a consequence of TAS being added later than this logic was written.
     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);
-        assertSame(r1, ret.second);
+        assertNotSame(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 92e94256411..89d5db62899 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,6 +155,16 @@ 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.
@@ -223,10 +233,10 @@ public class PolicyTestFrame {
 
         Map<String, Integer> replies = new HashMap<>();
         replies.put(recipient, ErrorCode.NONE);
-        assertMergeOk(replies, List.of(recipient));
+        assertMergeOk(replies, Arrays.asList(recipient));
 
         replies.put(recipient, ErrorCode.TRANSIENT_ERROR);
-        assertMerge(replies, List.of(ErrorCode.TRANSIENT_ERROR), List.of(recipient));
+        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR));
     }
 
     /**
@@ -242,33 +252,28 @@ public class PolicyTestFrame {
         Map<String, Integer> replies = new HashMap<>();
         replies.put(recipientOne, ErrorCode.NONE);
         replies.put(recipientTwo, ErrorCode.NONE);
-        assertMergeOk(replies, List.of(recipientOne, recipientTwo));
+        assertMergeOk(replies, Arrays.asList(recipientOne, recipientTwo));
 
         replies.put(recipientOne, ErrorCode.TRANSIENT_ERROR);
         replies.put(recipientTwo, ErrorCode.NONE);
-        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));
+        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR));
 
         replies.put(recipientOne, ErrorCode.TRANSIENT_ERROR);
         replies.put(recipientTwo, ErrorCode.TRANSIENT_ERROR);
-        assertMerge(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR, ErrorCode.TRANSIENT_ERROR), List.of(recipientOne, recipientTwo));
+        assertMergeError(replies, Arrays.asList(ErrorCode.TRANSIENT_ERROR, ErrorCode.TRANSIENT_ERROR));
 
         replies.put(recipientOne, ErrorCode.NONE);
         replies.put(recipientTwo, DocumentProtocol.ERROR_MESSAGE_IGNORED);
-        assertMergeOk(replies, List.of(recipientOne));
+        assertMergeOk(replies, Arrays.asList(recipientOne));
 
         replies.put(recipientOne, DocumentProtocol.ERROR_MESSAGE_IGNORED);
         replies.put(recipientTwo, ErrorCode.NONE);
-        assertMergeOk(replies, List.of(recipientTwo));
+        assertMergeOk(replies, Arrays.asList(recipientTwo));
 
         replies.put(recipientOne, DocumentProtocol.ERROR_MESSAGE_IGNORED);
         replies.put(recipientTwo, DocumentProtocol.ERROR_MESSAGE_IGNORED);
-        assertMerge(replies, List.of(DocumentProtocol.ERROR_MESSAGE_IGNORED,
-                                     DocumentProtocol.ERROR_MESSAGE_IGNORED),
-                    null); // Only ignored errors specifically causes an EmptyReply.
+        assertMergeError(replies, Arrays.asList(DocumentProtocol.ERROR_MESSAGE_IGNORED,
+                                                DocumentProtocol.ERROR_MESSAGE_IGNORED));
     }
 
     /**