modules/core/domain/src/main/java/org/rhq/core/domain/alert/AlertDefinition.java | 30 ++- modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/alert/definitions/TemplateAlertDefinitionsView.java | 4 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/AlertTemplateGWTService.java | 4 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/AlertTemplateGWTServiceImpl.java | 4 modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertDefinitionManagerBean.java | 4 modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertTemplateManagerBean.java | 6 modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/alert/test/AlertConditionTest.java | 78 +++++++++- 7 files changed, 109 insertions(+), 21 deletions(-)
New commits: commit 2c5974fbda6a05ab0d7d233c190740d45df067d5 Author: Jay Shaughnessy jshaughn@redhat.com Date: Fri Jun 22 16:09:36 2012 -0400
[830463 - Re-enabling (actually, updating) an alert definition clears condition logs for existing alerts] Some follow-on work. Add optimization such that if conditions are not being updated we don't detach the AlertCondition from the def, since they are unchanged. - Add related unit testing - re-enable the tests in AlertConditionTest, which pass but for some reason were not enabled. - Fix a few parameter names that I missed updating last time.
diff --git a/modules/core/domain/src/main/java/org/rhq/core/domain/alert/AlertDefinition.java b/modules/core/domain/src/main/java/org/rhq/core/domain/alert/AlertDefinition.java index 397e254..71ca46c 100644 --- a/modules/core/domain/src/main/java/org/rhq/core/domain/alert/AlertDefinition.java +++ b/modules/core/domain/src/main/java/org/rhq/core/domain/alert/AlertDefinition.java @@ -62,7 +62,7 @@ import org.rhq.core.domain.resource.group.ResourceGroup; * @author Joseph Marques */ @Entity -@NamedQueries( { // +@NamedQueries({ // @NamedQuery(name = AlertDefinition.QUERY_FIND_ALL, query = "" // + "SELECT a " // + " FROM AlertDefinition a " // @@ -409,6 +409,16 @@ public class AlertDefinition implements Serializable { }
public void update(AlertDefinition alertDef) { + update(alertDef, true); + } + + /** + * @param alertDef + * @param updateConditions set to true if conditions are being updated. This incurs overhead because the + * old AlertConditions get detached and replaced for no good reason, becoming a cleanup burden later on. Only + * set to false if you are sure the conditions are not being updated. If false the conditions will not be updated. + */ + public void update(AlertDefinition alertDef, boolean updateConditions) { /* * Don't copy the id, ctime, or mtime. */ @@ -440,15 +450,17 @@ public class AlertDefinition implements Serializable {
this.recoveryId = alertDef.recoveryId;
- // copy conditions - List<AlertCondition> copiedConditions = new ArrayList<AlertCondition>(); - for (AlertCondition oldCondition : alertDef.getConditions()) { - AlertCondition newCondition = new AlertCondition(oldCondition); - newCondition.setAlertDefinition(this); - copiedConditions.add(newCondition); + // copy conditions if necessary + if (updateConditions) { + List<AlertCondition> copiedConditions = new ArrayList<AlertCondition>(); + for (AlertCondition oldCondition : alertDef.getConditions()) { + AlertCondition newCondition = new AlertCondition(oldCondition); + newCondition.setAlertDefinition(this); + copiedConditions.add(newCondition); + } + this.removeAllConditions(); + this.getConditions().addAll(copiedConditions); } - this.removeAllConditions(); - this.getConditions().addAll(copiedConditions);
// copy notifications List<AlertNotification> copiedNotifications = new ArrayList<AlertNotification>(); diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/alert/definitions/TemplateAlertDefinitionsView.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/alert/definitions/TemplateAlertDefinitionsView.java index 987e62f..0c710b4 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/alert/definitions/TemplateAlertDefinitionsView.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/alert/definitions/TemplateAlertDefinitionsView.java @@ -206,7 +206,7 @@ public class TemplateAlertDefinitionsView extends AbstractAlertDefinitionsView { }
@Override - protected void commitAlertDefinition(final AlertDefinition alertDefinition, boolean purgeInternals) { + protected void commitAlertDefinition(final AlertDefinition alertDefinition, boolean resetMatching) { if (alertDefinition.getId() == 0) { GWTServiceLookup.getAlertTemplateService().createAlertTemplate(alertDefinition, Integer.valueOf(this.resourceType.getId()), new AsyncCallback<Integer>() { @@ -224,7 +224,7 @@ public class TemplateAlertDefinitionsView extends AbstractAlertDefinitionsView { } }); } else { - GWTServiceLookup.getAlertTemplateService().updateAlertTemplate(alertDefinition, purgeInternals, + GWTServiceLookup.getAlertTemplateService().updateAlertTemplate(alertDefinition, resetMatching, new AsyncCallback<AlertDefinition>() { @Override public void onSuccess(AlertDefinition result) { diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/AlertTemplateGWTService.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/AlertTemplateGWTService.java index e9af2cc..0a7f27b 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/AlertTemplateGWTService.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/AlertTemplateGWTService.java @@ -30,11 +30,11 @@ public interface AlertTemplateGWTService extends RemoteService { * Updates a alert template definition. * * @param alertDefinition - * @param purgeInternals must be true if you are updating conditions or dampening settings, can be false otherwise + * @param resetMatching must be true if you are updating conditions, condition expression, or dampening settings, can be false otherwise * @return the updated definition * @throws Exception */ - AlertDefinition updateAlertTemplate(AlertDefinition alertDefinition, boolean purgeInternals) + AlertDefinition updateAlertTemplate(AlertDefinition alertDefinition, boolean resetMatching) throws RuntimeException;
void enableAlertTemplates(Integer[] alertDefinitionIds) throws RuntimeException; diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/AlertTemplateGWTServiceImpl.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/AlertTemplateGWTServiceImpl.java index ceddfa7..7e1fd44 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/AlertTemplateGWTServiceImpl.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/AlertTemplateGWTServiceImpl.java @@ -41,11 +41,11 @@ public class AlertTemplateGWTServiceImpl extends AbstractGWTServiceImpl implemen }
@Override - public AlertDefinition updateAlertTemplate(AlertDefinition alertDefinition, boolean purgeInternals) + public AlertDefinition updateAlertTemplate(AlertDefinition alertDefinition, boolean resetMatching) throws RuntimeException { try { AlertDefinition results = alertTemplateManager.updateAlertTemplate(getSessionSubject(), alertDefinition, - purgeInternals); + resetMatching); return SerialUtility.prepare(results, "AlertTemplateService.updateAlertTemplate"); } catch (Throwable t) { throw getExceptionToThrowToClient(t); diff --git a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertDefinitionManagerBean.java b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertDefinitionManagerBean.java index 5da131e..c4fdcda 100644 --- a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertDefinitionManagerBean.java +++ b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertDefinitionManagerBean.java @@ -60,8 +60,8 @@ import org.rhq.enterprise.server.cloud.StatusManagerLocal; import org.rhq.enterprise.server.plugin.pc.alert.AlertSender; import org.rhq.enterprise.server.plugin.pc.alert.AlertSenderPluginManager; import org.rhq.enterprise.server.util.CriteriaQueryGenerator; -import org.rhq.enterprise.server.util.CriteriaQueryRunner; import org.rhq.enterprise.server.util.CriteriaQueryGenerator.AuthorizationTokenType; +import org.rhq.enterprise.server.util.CriteriaQueryRunner;
/** * @author Joseph Marques @@ -519,7 +519,7 @@ public class AlertDefinitionManagerBean implements AlertDefinitionManagerLocal, alertDefinition.setConditionExpression(BooleanExpression.ANY); }
- oldAlertDefinition.update(alertDefinition); + oldAlertDefinition.update(alertDefinition, resetMatching); if (LOG.isDebugEnabled()) { LOG.debug("Updating: " + oldAlertDefinition); for (AlertCondition nextCondition : oldAlertDefinition.getConditions()) { diff --git a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertTemplateManagerBean.java b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertTemplateManagerBean.java index 94f1d35..5b4ad77 100644 --- a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertTemplateManagerBean.java +++ b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/alert/AlertTemplateManagerBean.java @@ -231,7 +231,7 @@ public class AlertTemplateManagerBean implements AlertTemplateManagerLocal {
@RequiredPermission(Permission.MANAGE_SETTINGS) @TransactionAttribute(TransactionAttributeType.NOT_SUPPORTED) - public AlertDefinition updateAlertTemplate(Subject user, AlertDefinition alertTemplate, boolean purgeInternals) + public AlertDefinition updateAlertTemplate(Subject user, AlertDefinition alertTemplate, boolean resetMatching) throws InvalidAlertDefinitionException, AlertDefinitionUpdateException, AlertNotificationValidationException { if (LOG.isDebugEnabled()) { LOG.debug("updateAlertTemplate: " + alertTemplate); @@ -241,7 +241,7 @@ public class AlertTemplateManagerBean implements AlertTemplateManagerLocal { AlertDefinition updated = null; try { updated = alertDefinitionManager.updateAlertDefinition(user, alertTemplate.getId(), alertTemplate, - purgeInternals); // do not allow direct undeletes of an alert definition + resetMatching); // do not allow direct undeletes of an alert definition } catch (Throwable t) { throw new AlertDefinitionUpdateException("Failed to update an AlertTemplate " + alertTemplate.toSimpleString(), t); @@ -260,7 +260,7 @@ public class AlertTemplateManagerBean implements AlertTemplateManagerLocal { for (Integer alertDefinitionId : alertDefinitions) { try { alertDefinitionManager - .updateAlertDefinition(overlord, alertDefinitionId, alertTemplate, purgeInternals); + .updateAlertDefinition(overlord, alertDefinitionId, alertTemplate, resetMatching); } catch (Throwable t) { // continue on error, update as many as possible if (firstThrowable == null) { diff --git a/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/alert/test/AlertConditionTest.java b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/alert/test/AlertConditionTest.java index 645d049..47ba274 100644 --- a/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/alert/test/AlertConditionTest.java +++ b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/alert/test/AlertConditionTest.java @@ -20,12 +20,14 @@ package org.rhq.enterprise.server.alert.test;
import java.util.HashSet; +import java.util.List; import java.util.Set;
import javax.persistence.EntityManager; import javax.persistence.NoResultException; import javax.transaction.TransactionManager;
+import org.testng.Assert; import org.testng.annotations.AfterClass; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; @@ -70,7 +72,7 @@ import org.rhq.test.TransactionCallbackWithContext;
@Test public class AlertConditionTest extends UpdatePluginMetadataTestBase { - private static final boolean ENABLED = false; + private static final boolean ENABLED = true;
// this must match the constant found in ServerManagerBean private static final String RHQ_SERVER_NAME_PROPERTY = "rhq.server.high-availability.name"; @@ -343,10 +345,84 @@ public class AlertConditionTest extends UpdatePluginMetadataTestBase { } }
+ @Test(enabled = ENABLED) + public void testBZ830463_updateDef() throws Exception { + // create our resource with alert definition + MeasurementDefinition metricDef = createResourceWithMetricSchedule(); + AlertDefinition alertDef = createAlertDefinitionWithOneInsideRangeCondition(metricDef, resource.getId()); + + assert alertDef.getConditions().size() == 1 : "1 alertDef condition should exist"; + AlertCondition condition = alertDef.getConditions().iterator().next(); + int conditionId = condition.getId(); + + // re-load the resource so we get the measurement schedule + Resource resourceWithSchedules = loadResourceWithSchedules(resource.getId()); + MeasurementSchedule schedule = resourceWithSchedules.getSchedules().iterator().next(); + + // simulate a measurement report coming from the agent - one values that fits in our range, so 1 alert is fired + MeasurementScheduleRequest request = new MeasurementScheduleRequest(schedule); + MeasurementReport report = new MeasurementReport(); + report.addData(new MeasurementDataNumeric(getTimestamp(15), request, Double.valueOf(50.0))); // 50 < 60 AND 50 > 40 + MeasurementDataManagerLocal dataManager = LookupUtil.getMeasurementDataManager(); + dataManager.mergeMeasurementReport(report); + + // wait for our JMS messages to process and see if we get any alerts + Thread.sleep(5000); + + // make sure one alert was triggered + List<Alert> alerts = getAlerts(resourceWithSchedules.getId()); + assert alerts.size() == 1 : "1 alert should have fired: " + alerts; + Alert alert = alerts.get(0); + + assert alert.getConditionLogs().size() == 1 : "1 condition log should exist"; + AlertConditionLog conditionLog = alert.getConditionLogs().iterator().next(); + Assert.assertEquals(conditionLog.getCondition().getId(), conditionId, + "original condition should have been associated with the alert"); + + // update a non-condition aspect of the def and then update the def + String updatedDesc = "Updated Description"; + alertDef.setDescription(updatedDesc); + AlertDefinition updatedAlertDef = LookupUtil.getAlertDefinitionManager().updateAlertDefinition(getOverlord(), + alertDef.getId(), alertDef, false); // note that resetMatching is false + assert updatedDesc.equals(updatedAlertDef.getDescription()) : "Description should be updated"; + assert updatedAlertDef.getConditions().size() == 1 : "1 alertDef condition should exist after the update"; + assert updatedAlertDef.getConditions().iterator().next().getId() == condition.getId() : "condition should not be updated"; + + // get the alert again, and make sure it still has a log with the same condition + alerts = getAlerts(resourceWithSchedules.getId()); + assert alerts.size() == 1 : "1 alert should have fired: " + alerts; + alert = alerts.get(0); + + assert alert.getConditionLogs().size() == 1 : "1 condition log should exist after the update"; + conditionLog = alert.getConditionLogs().iterator().next(); + Assert.assertEquals(conditionLog.getCondition().getId(), conditionId, + "original condition should still have been associated with the alert"); + + // update the condition on the def and then update the def + condition.setThreshold(Double.valueOf(41.0)); + updatedAlertDef = LookupUtil.getAlertDefinitionManager().updateAlertDefinition(getOverlord(), alertDef.getId(), + alertDef, true); // note that resetMatching is true + assert updatedAlertDef.getConditions().size() == 1 : "1 alertDef condition should exist after the update"; + assert updatedAlertDef.getConditions().iterator().next().getId() != condition.getId() : "condition should be updated"; + + // get the alert again, and make sure it still has a log with the same condition + alerts = getAlerts(resourceWithSchedules.getId()); + assert alerts.size() == 1 : "1 alert should have fired: " + alerts; + alert = alerts.get(0); + + assert alert.getConditionLogs().size() == 1 : "1 condition log should exist after the update"; + conditionLog = alert.getConditionLogs().iterator().next(); + Assert.assertEquals(conditionLog.getCondition().getId(), conditionId, + "original condition should still have been associated with the alert"); + + return; + } + private PageList<Alert> getAlerts(int resourceId) { AlertManagerLocal alertManager = LookupUtil.getAlertManager(); AlertCriteria alertCriteria = new AlertCriteria(); alertCriteria.addFilterResourceIds(resourceId); + alertCriteria.fetchConditionLogs(true); PageList<Alert> alerts = alertManager.findAlertsByCriteria(getOverlord(), alertCriteria); return alerts; }