From d71b4e3cc240e350ff146554b3f3d8399d03a669 Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Mon, 24 Feb 2020 10:38:07 -0500 Subject: [PATCH] Add "finalOutcome" flag to OperationOutcome The Actor clients receive notifications of operation failures, but no indication if it's the final failure, thus they do not know if they should progress on to the "failure" policies. Issue-ID: POLICY-2385 Signed-off-by: Jim Hahn Change-Id: If053ed459e1d790e58eca950a8feeabaf4d67a41 --- .../actorserviceprovider/OperationOutcome.java | 2 + .../impl/OperationPartial.java | 65 ++++++++++++++-------- .../impl/OperationPartialTest.java | 38 +++++++++++-- 3 files changed, 76 insertions(+), 29 deletions(-) diff --git a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/OperationOutcome.java b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/OperationOutcome.java index 6b0924807..d8db70619 100644 --- a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/OperationOutcome.java +++ b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/OperationOutcome.java @@ -41,6 +41,7 @@ public class OperationOutcome { private String subRequestId; private PolicyResult result = PolicyResult.SUCCESS; private String message; + private boolean finalOutcome; /** * Copy constructor. @@ -56,6 +57,7 @@ public class OperationOutcome { this.subRequestId = source.subRequestId; this.result = source.result; this.message = source.message; + this.finalOutcome = source.finalOutcome; } /** diff --git a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java index c998209bc..be357b8f3 100644 --- a/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java +++ b/models-interactions/model-actors/actorServiceProvider/src/main/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartial.java @@ -175,6 +175,7 @@ public abstract class OperationPartial implements Operation { // TODO need a FAILURE_MISSING_DATA (e.g., A&AI) + outcome2.setFinalOutcome(true); outcome2.setResult(PolicyResult.FAILURE_GUARD); outcome2.setMessage(outcome != null ? outcome.getMessage() : null); @@ -220,13 +221,13 @@ public abstract class OperationPartial implements Operation { */ protected CompletableFuture startGuardAsync() { // get the guard payload - Map guardPayload = makeGuardPayload(); + Map guardPayload = makeGuardPayload(); // wrap it in a "resource" - Map resource = new LinkedHashMap<>(); + Map resource = new LinkedHashMap<>(); resource.put("guard", guardPayload); - Map payload = new LinkedHashMap<>(); + Map payload = new LinkedHashMap<>(); payload.put("resource", resource); /* @@ -411,35 +412,47 @@ public abstract class OperationPartial implements Operation { */ private Function setRetryFlag(int attempt) { - return operation -> { - if (operation != null && !isActorFailed(operation)) { - /* - * wrong type or wrong operation - just leave it as is. No need to log - * anything here, as retryOnFailure() will log a message - */ - return operation; + return origOutcome -> { + // ensure we have a non-null outcome + OperationOutcome outcome; + if (origOutcome != null) { + outcome = origOutcome; + } else { + logger.warn("{}: null outcome; treating as a failure for {}", getFullName(), params.getRequestId()); + outcome = this.setOutcome(params.makeOutcome(), PolicyResult.FAILURE); } - // get a non-null operation - OperationOutcome oper2; - if (operation != null) { - oper2 = operation; - } else { - oper2 = params.makeOutcome(); - oper2.setResult(PolicyResult.FAILURE); + // ensure correct actor/operation + outcome.setActor(getActorName()); + outcome.setOperation(getName()); + + // determine if we should retry, based on the result + if (outcome.getResult() != PolicyResult.FAILURE) { + // do not retry success or other failure types (e.g., exception) + outcome.setFinalOutcome(true); + return outcome; } int retry = getRetry(params.getRetry()); - if (retry > 0 && attempt > retry) { + if (retry <= 0) { + // no retries were specified + outcome.setFinalOutcome(true); + + } else if (attempt <= retry) { + // have more retries - not the final outcome + outcome.setFinalOutcome(false); + + } else { /* * retries were specified and we've already tried them all - change to * FAILURE_RETRIES */ logger.info("operation {} retries exhausted for {}", getFullName(), params.getRequestId()); - oper2.setResult(PolicyResult.FAILURE_RETRIES); + outcome.setResult(PolicyResult.FAILURE_RETRIES); + outcome.setFinalOutcome(true); } - return oper2; + return outcome; }; } @@ -870,10 +883,13 @@ public abstract class OperationPartial implements Operation { return (outcome, thrown) -> { if (callbacks.canStart()) { - // haven't invoked "start" callback yet outcome.setStart(callbacks.getStartTime()); outcome.setEnd(null); - params.callbackStarted(outcome); + + // pass a copy to the callback + OperationOutcome outcome2 = new OperationOutcome(outcome); + outcome2.setFinalOutcome(false); + params.callbackStarted(outcome2); } }; } @@ -894,11 +910,12 @@ public abstract class OperationPartial implements Operation { private BiConsumer callbackCompleted(CallbackManager callbacks) { return (outcome, thrown) -> { - if (callbacks.canEnd()) { outcome.setStart(callbacks.getStartTime()); outcome.setEnd(callbacks.getEndTime()); - params.callbackCompleted(outcome); + + // pass a copy to the callback + params.callbackCompleted(new OperationOutcome(outcome)); } }; } diff --git a/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java b/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java index 2893cb627..229e5a32a 100644 --- a/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java +++ b/models-interactions/model-actors/actorServiceProvider/src/test/java/org/onap/policy/controlloop/actorserviceprovider/impl/OperationPartialTest.java @@ -34,7 +34,9 @@ import static org.mockito.Mockito.when; import ch.qos.logback.classic.Logger; import java.time.Instant; +import java.util.ArrayDeque; import java.util.Arrays; +import java.util.Deque; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -128,6 +130,9 @@ public class OperationPartialTest { private OperationOutcome opstart; private OperationOutcome opend; + private Deque starts; + private Deque ends; + private OperatorConfig config; /** @@ -182,6 +187,9 @@ public class OperationPartialTest { opstart = null; opend = null; + + starts = new ArrayDeque<>(10); + ends = new ArrayDeque<>(10); } @Test @@ -519,10 +527,10 @@ public class OperationPartialTest { // arrange to return null from doOperation() oper = new MyOper() { @Override - protected OperationOutcome doOperation(int attempt, OperationOutcome operation) { + protected OperationOutcome doOperation(int attempt, OperationOutcome outcome) { // update counters - super.doOperation(attempt, operation); + super.doOperation(attempt, outcome); return null; } }; @@ -587,7 +595,7 @@ public class OperationPartialTest { * Tests handleFailure() when the outcome is a success. */ @Test - public void testHandlePreprocessorFailureTrue() { + public void testHandlePreprocessorFailureSuccess() { oper.setPreProc(CompletableFuture.completedFuture(makeSuccess())); verifyRun("testHandlePreprocessorFailureTrue", 1, 1, PolicyResult.SUCCESS); } @@ -596,7 +604,7 @@ public class OperationPartialTest { * Tests handleFailure() when the outcome is not a success. */ @Test - public void testHandlePreprocessorFailureFalse() throws Exception { + public void testHandlePreprocessorFailureFailed() throws Exception { oper.setPreProc(CompletableFuture.completedFuture(makeFailure())); verifyRun("testHandlePreprocessorFailureFalse", 1, 0, PolicyResult.FAILURE_GUARD); } @@ -1130,11 +1138,13 @@ public class OperationPartialTest { ++numStart; tstart = oper.getStart(); opstart = oper; + starts.add(oper); } private void completer(OperationOutcome oper) { ++numEnd; opend = oper; + ends.add(oper); } /** @@ -1194,6 +1204,12 @@ public class OperationPartialTest { private void verifyRun(String testName, int expectedCallbacks, int expectedOperations, PolicyResult expectedResult, String expectedSubRequestId, Consumer> manipulator) { + tstart = null; + opstart = null; + opend = null; + starts.clear(); + ends.clear(); + CompletableFuture future = oper.start(); manipulator.accept(future); @@ -1213,7 +1229,19 @@ public class OperationPartialTest { try { assertTrue(future.isDone()); - assertSame(testName, opend, future.get()); + assertEquals(testName, opend, future.get()); + + // "start" is never final + for (OperationOutcome outcome : starts) { + assertFalse(testName, outcome.isFinalOutcome()); + } + + // only the last "complete" is final + assertTrue(testName, ends.removeLast().isFinalOutcome()); + + for (OperationOutcome outcome : ends) { + assertFalse(outcome.isFinalOutcome()); + } } catch (InterruptedException | ExecutionException e) { throw new IllegalStateException(e); -- 2.16.6