diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2021-01-07 06:52:22 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-01-07 06:52:22 +0100 |
commit | 9263966a9c697f5acff53a595493e27a2f05e683 (patch) | |
tree | c6a85dc50a71b2fb0b6f21ccc5da3a3c80db7ae0 /documentapi/src | |
parent | 84d629c5125f2e0afc93ed730875215cb68f1f38 (diff) |
Revert "Keep reply with most severe error when merging error replies"
Diffstat (limited to 'documentapi/src')
4 files changed, 36 insertions, 39 deletions
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 2680ed011af..f5b4920fa3f 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<>(0)); + merge(ctx, new HashSet<Integer>(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<>(0)).second; + return merge(replies, new HashSet<Integer>(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 630a8588495..a8ec53c5c97 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,7 +3,6 @@ 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; /** @@ -58,22 +57,15 @@ final class ReplyMerger { return; } if (error == null) { - error = r; - } - else if (mostSevereErrorCode(r) > mostSevereErrorCode(error)) { - error.getErrors().forEach(r::addError); - error = r; + error = new EmptyReply(); + r.swapState(error); + return; } - else { - r.getErrors().forEach(error::addError); + for (int j = 0; j < r.getNumErrors(); ++j) { + error.addError(r.getError(j)); } } - 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) { @@ -104,7 +96,7 @@ final class ReplyMerger { } private Tuple2<Integer, Reply> createEmptyReplyResult() { - return new Tuple2<>(null, new EmptyReply()); + return new Tuple2<>(null, (Reply)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 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)); } /** |