From d0932a1a2339a02dab04eedefa0480535e68d31c Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Thu, 27 Jun 2019 10:52:06 -0400 Subject: Fix some sonar issues in drools-applications Added coverage to: - feature-controlloop-management Fixed sonar issues, but didn't add coverage to: - feature-controlloop-trans - eventmanager - guard Change-Id: I12f09c4a533e838c6fb9762ba56194e51ce864eb Issue-ID: POLICY-1791 Signed-off-by: Jim Hahn --- .../trans/CacheBasedControlLoopMetricsManager.java | 73 +++++++++++++--------- .../trans/ControlLoopMetricsFeatureTest.java | 43 ++++++------- 2 files changed, 61 insertions(+), 55 deletions(-) (limited to 'controlloop/common/feature-controlloop-trans/src') diff --git a/controlloop/common/feature-controlloop-trans/src/main/java/org/onap/policy/drools/apps/controlloop/feature/trans/CacheBasedControlLoopMetricsManager.java b/controlloop/common/feature-controlloop-trans/src/main/java/org/onap/policy/drools/apps/controlloop/feature/trans/CacheBasedControlLoopMetricsManager.java index c5d6a32ac..672ff3fba 100644 --- a/controlloop/common/feature-controlloop-trans/src/main/java/org/onap/policy/drools/apps/controlloop/feature/trans/CacheBasedControlLoopMetricsManager.java +++ b/controlloop/common/feature-controlloop-trans/src/main/java/org/onap/policy/drools/apps/controlloop/feature/trans/CacheBasedControlLoopMetricsManager.java @@ -24,7 +24,6 @@ import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.cache.RemovalListener; -import com.google.common.cache.RemovalNotification; import java.time.Instant; import java.time.ZonedDateTime; @@ -47,6 +46,8 @@ import org.slf4j.LoggerFactory; */ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { + private static final String UNEXPECTED_NOTIFICATION_TYPE = "unexpected notification type {} in notification {}"; + private static final Logger logger = LoggerFactory.getLogger(CacheBasedControlLoopMetricsManager.class); private LoadingCache cache; @@ -98,16 +99,12 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { } }; - RemovalListener listener = - new RemovalListener() { - @Override - public void onRemoval(RemovalNotification notification) { - if (notification.wasEvicted()) { - evicted(notification.getValue()); - } else { - logger.info("REMOVAL: {} because of {}", notification.getValue().getRequestId(), - notification.getCause().name()); - } + RemovalListener listener = notification -> { + if (notification.wasEvicted()) { + evicted(notification.getValue()); + } else { + logger.info("REMOVAL: {} because of {}", notification.getValue().getRequestId(), + notification.getCause().name()); } }; @@ -139,16 +136,11 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { @Override public void transactionEvent(PolicyController controller, VirtualControlLoopNotification notification) { - if (notification == null || notification.getRequestId() == null || notification.getNotification() == null) { - logger.warn("Invalid notification: {}", notification); + if (!isNotificationValid(notification)) { return; } - if (notification.getNotificationTime() == null) { - notification.setNotificationTime(ZonedDateTime.now()); - } - - notification.setFrom(notification.getFrom() + ":" + controller.getName()); + setNotificationValues(controller, notification); switch (notification.getNotification()) { case REJECTED: @@ -166,12 +158,29 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { break; default: /* unexpected */ - logger.warn("unexpected notification type {} in notification {}", - notification.getNotification().toString(), notification); + logger.warn(UNEXPECTED_NOTIFICATION_TYPE, + notification.getNotification(), notification); break; } } + private boolean isNotificationValid(VirtualControlLoopNotification notification) { + if (notification == null || notification.getRequestId() == null || notification.getNotification() == null) { + logger.warn("Invalid notification: {}", notification); + return false; + } + + return true; + } + + private void setNotificationValues(PolicyController controller, VirtualControlLoopNotification notification) { + if (notification.getNotificationTime() == null) { + notification.setNotificationTime(ZonedDateTime.now()); + } + + notification.setFrom(notification.getFrom() + ":" + controller.getName()); + } + @Override public VirtualControlLoopNotification getTransaction(UUID requestId) { return cache.getIfPresent(requestId); @@ -260,13 +269,7 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { trans.metric().resetTransaction(); break; case OPERATION: - trans.setStatusCode(true); - if (!operations.isEmpty()) { - ControlLoopOperation operation = operations.get(operations.size() - 1); - trans.setTargetEntity(operation.getTarget()); - trans.setTargetServiceName(operation.getActor()); - } - trans.metric().resetTransaction(); + metricOperation(trans, operations); break; case OPERATION_SUCCESS: trans.setStatusCode(true); @@ -280,12 +283,22 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { break; default: /* unexpected */ - logger.warn("unexpected notification type {} in notification {}", - notification.getNotification().toString(), notification); + logger.warn(UNEXPECTED_NOTIFICATION_TYPE, + notification.getNotification(), notification); break; } } + private void metricOperation(MDCTransaction trans, List operations) { + trans.setStatusCode(true); + if (!operations.isEmpty()) { + ControlLoopOperation operation = operations.get(operations.size() - 1); + trans.setTargetEntity(operation.getTarget()); + trans.setTargetServiceName(operation.getActor()); + } + trans.metric().resetTransaction(); + } + protected void operation(MDCTransaction trans, List operations) { if (!operations.isEmpty()) { ControlLoopOperation operation = operations.get(operations.size() - 1); @@ -340,7 +353,7 @@ class CacheBasedControlLoopMetricsManager implements ControlLoopMetrics { break; default: /* unexpected */ - logger.warn("unexpected notification type {} in notification {}", + logger.warn(UNEXPECTED_NOTIFICATION_TYPE, notification.getNotification(), notification); break; } diff --git a/controlloop/common/feature-controlloop-trans/src/test/java/org/onap/policy/drools/apps/controlloop/feature/trans/ControlLoopMetricsFeatureTest.java b/controlloop/common/feature-controlloop-trans/src/test/java/org/onap/policy/drools/apps/controlloop/feature/trans/ControlLoopMetricsFeatureTest.java index 1f1e9de39..661803315 100644 --- a/controlloop/common/feature-controlloop-trans/src/test/java/org/onap/policy/drools/apps/controlloop/feature/trans/ControlLoopMetricsFeatureTest.java +++ b/controlloop/common/feature-controlloop-trans/src/test/java/org/onap/policy/drools/apps/controlloop/feature/trans/ControlLoopMetricsFeatureTest.java @@ -2,7 +2,7 @@ * ============LICENSE_START======================================================= * ONAP * ================================================================================ - * Copyright (C) 2018 AT&T Intellectual Property. All rights reserved. + * Copyright (C) 2018-2019 AT&T Intellectual Property. All rights reserved. * ================================================================================ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ package org.onap.policy.drools.apps.controlloop.feature.trans; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; @@ -27,7 +28,6 @@ import static org.junit.Assert.assertTrue; import java.nio.file.Path; import java.util.UUID; - import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; @@ -43,6 +43,7 @@ import org.onap.policy.drools.system.PolicyEngine; */ public class ControlLoopMetricsFeatureTest { + private static final String POLICY_CL_MGT = "POLICY-CL-MGT"; private static final Path configPath = SystemPersistence.manager.getConfigurationPath(); private static PolicyController testController; @@ -63,46 +64,42 @@ public class ControlLoopMetricsFeatureTest { @Test public void cacheDefaults() { - assertTrue(ControlLoopMetrics.manager.getCacheSize() == 3); - assertTrue(ControlLoopMetrics.manager.getTransactionTimeout() == 10); - assertTrue(ControlLoopMetrics.manager.getCacheOccupancy() == 0); + assertEquals(3, ControlLoopMetrics.manager.getCacheSize()); + assertEquals(10, ControlLoopMetrics.manager.getTransactionTimeout()); + assertEquals(0, ControlLoopMetrics.manager.getCacheOccupancy()); } @Test public void invalidNotifications() { ControlLoopMetricsFeature feature = new ControlLoopMetricsFeature(); VirtualControlLoopNotification notification = new VirtualControlLoopNotification(); - feature.beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", notification); + feature.beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, notification); this.cacheDefaults(); UUID requestId = UUID.randomUUID(); notification.setRequestId(requestId); - feature.beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", notification); + feature.beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, notification); assertNull(ControlLoopMetrics.manager.getTransaction(requestId)); this.cacheDefaults(); } @Test - public void validActiveNotification() { + public void validActiveNotification() throws InterruptedException { ControlLoopMetricsFeature feature = new ControlLoopMetricsFeature(); VirtualControlLoopNotification notification = new VirtualControlLoopNotification(); UUID requestId = UUID.randomUUID(); notification.setRequestId(requestId); notification.setNotification(ControlLoopNotificationType.ACTIVE); - feature.beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", notification); + feature.beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, notification); assertNotNull(ControlLoopMetrics.manager.getTransaction(requestId)); assertTrue(ControlLoopMetrics.manager.getTransaction(requestId).getFrom().contains(testController.getName())); assertNotNull(ControlLoopMetrics.manager.getTransaction(requestId).getNotificationTime()); assertTrue(ControlLoopMetrics.manager.getCacheOccupancy() == 1); /* let the entries expire */ - try { - Thread.sleep((ControlLoopMetrics.manager.getTransactionTimeout() + 5) * 1000L); - } catch (InterruptedException e) { - /* nothing to do */ - } + Thread.sleep((ControlLoopMetrics.manager.getTransactionTimeout() + 1) * 1000L); assertNull(ControlLoopMetrics.manager.getTransaction(requestId)); this.cacheDefaults(); @@ -111,7 +108,7 @@ public class ControlLoopMetricsFeatureTest { @Test public void reset() { VirtualControlLoopNotification notification = this.generateNotification(); - new ControlLoopMetricsFeature().beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", + new ControlLoopMetricsFeature().beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, notification); assertNotNull(ControlLoopMetrics.manager.getTransaction(notification.getRequestId())); @@ -135,19 +132,19 @@ public class ControlLoopMetricsFeatureTest { } @Test - public void eviction() { + public void eviction() throws InterruptedException { ControlLoopMetricsFeature feature = new ControlLoopMetricsFeature(); for (int i = 0; i < ControlLoopMetrics.manager.getCacheSize(); i++) { VirtualControlLoopNotification notification = generateNotification(); - feature.beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", notification); + feature.beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, notification); assertNotNull(ControlLoopMetrics.manager.getTransaction(notification.getRequestId())); } - assertTrue(ControlLoopMetrics.manager.getCacheOccupancy() == ControlLoopMetrics.manager.getCacheOccupancy()); + assertEquals(ControlLoopMetrics.manager.getCacheOccupancy(), ControlLoopMetrics.manager.getCacheOccupancy()); VirtualControlLoopNotification overflowNotification = generateNotification(); - feature.beforeDeliver(testController, CommInfrastructure.DMAAP, "POLICY-CL-MGT", overflowNotification); - assertTrue(ControlLoopMetrics.manager.getCacheOccupancy() == ControlLoopMetrics.manager.getCacheOccupancy()); + feature.beforeDeliver(testController, CommInfrastructure.DMAAP, POLICY_CL_MGT, overflowNotification); + assertEquals(ControlLoopMetrics.manager.getCacheOccupancy(), ControlLoopMetrics.manager.getCacheOccupancy()); assertNotNull(ControlLoopMetrics.manager.getTransaction(overflowNotification.getRequestId())); assertTrue(ControlLoopMetrics.manager.getTransactionIds().size() == ControlLoopMetrics.manager.getCacheSize()); assertTrue(ControlLoopMetrics.manager.getCacheOccupancy() == ControlLoopMetrics.manager.getCacheSize()); @@ -155,11 +152,7 @@ public class ControlLoopMetricsFeatureTest { assertFalse(ControlLoopMetrics.manager.getTransactions().isEmpty()); /* let the entries expire */ - try { - Thread.sleep((ControlLoopMetrics.manager.getTransactionTimeout() + 5) * 1000L); - } catch (InterruptedException e) { - /* nothing to do */ - } + Thread.sleep((ControlLoopMetrics.manager.getTransactionTimeout() + 1) * 1000L); ControlLoopMetrics.manager.refresh(); assertTrue(ControlLoopMetrics.manager.getTransactionIds().size() == ControlLoopMetrics.manager -- cgit 1.2.3-korg