From 0c675bb75556b4de4abdbf33149c70a01447d5d6 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 6 Jan 2021 11:42:15 +0100 Subject: Keep reply with most severe error when merging error replies --- .../messagebus/protocol/DocumentProtocol.java | 4 +-- .../messagebus/protocol/ReplyMerger.java | 20 +++++++++---- .../messagebus/protocol/ReplyMergerTestCase.java | 18 ++++++------ .../messagebus/protocol/test/PolicyTestFrame.java | 33 +++++++++------------- 4 files changed, 39 insertions(+), 36 deletions(-) (limited to 'documentapi/src') 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(0)); + merge(ctx, new HashSet<>(0)); } /** @@ -475,7 +475,7 @@ public class DocumentProtocol implements Protocol { * @return the merged Reply */ public static Reply merge(List replies) { - return merge(replies, new HashSet(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 createEmptyReplyResult() { - return new Tuple2<>(null, (Reply)new EmptyReply()); + return new Tuple2<>(null, new EmptyReply()); } public Tuple2 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 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 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 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 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 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 @@ -154,16 +154,6 @@ public class PolicyTestFrame { assertNotNull(handler.getReply(60)); } - /** - * 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 replies, List expectedErrors) { - assertMerge(replies, expectedErrors, null); - } - /** * This is a convenience method for invoking {@link this#assertMerge(Map,List,List)} with no expected errors. * @@ -233,10 +223,10 @@ public class PolicyTestFrame { Map 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 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. } /** -- cgit v1.2.3