AMBARI-19259 - When Updating An Alert Group a ConcurrentModificationException is Thrown (jonathanhurley)
Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/1584984e Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/1584984e Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/1584984e Branch: refs/heads/branch-dev-patch-upgrade Commit: 1584984e8836f6951794b3ac0309494cc9acb234 Parents: 4fc52a6 Author: Jonathan Hurley <[email protected]> Authored: Tue Dec 20 15:17:28 2016 -0500 Committer: Jonathan Hurley <[email protected]> Committed: Tue Dec 20 16:08:28 2016 -0500 ---------------------------------------------------------------------- .../server/orm/entities/AlertGroupEntity.java | 30 +++++++++--- .../server/orm/entities/AlertTargetEntity.java | 28 +++++++---- .../server/orm/dao/AlertDispatchDAOTest.java | 51 ++++++++++++++++++-- 3 files changed, 87 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/1584984e/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertGroupEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertGroupEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertGroupEntity.java index 76c6b62..b660631 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertGroupEntity.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertGroupEntity.java @@ -19,6 +19,7 @@ package org.apache.ambari.server.orm.entities; import java.util.Collections; import java.util.HashSet; +import java.util.Objects; import java.util.Set; import javax.persistence.CascadeType; @@ -337,19 +338,24 @@ public class AlertGroupEntity { * the targets, or {@code null} if there are none. */ public void setAlertTargets(Set<AlertTargetEntity> alertTargets) { + // for any existing associations, remove "this" from those associations if (null != this.alertTargets) { - for (AlertTargetEntity target : this.alertTargets) { + // make a copy to prevent ConcurrentModificiationExceptions + Set<AlertTargetEntity> copyOfAssociatedTargets = new HashSet<>(this.alertTargets); + for (AlertTargetEntity target : copyOfAssociatedTargets) { target.removeAlertGroup(this); } } - this.alertTargets = alertTargets; - + // update all new targets to reflect "this" as an associated group if (null != alertTargets) { for (AlertTargetEntity target : alertTargets) { target.addAlertGroup(this); } } + + // update reference + this.alertTargets = alertTargets; } /** @@ -367,11 +373,15 @@ public class AlertGroupEntity { AlertGroupEntity that = (AlertGroupEntity) object; - if (groupId != null ? !groupId.equals(that.groupId) : that.groupId != null) { - return false; + // use the unique ID if it exists + if( null != groupId ){ + return Objects.equals(groupId, that.groupId); } - return true; + return Objects.equals(groupId, that.groupId) && + Objects.equals(clusterId, that.clusterId) && + Objects.equals(groupName, that.groupName) && + Objects.equals(serviceName, that.serviceName); } /** @@ -379,8 +389,12 @@ public class AlertGroupEntity { */ @Override public int hashCode() { - int result = null != groupId ? groupId.hashCode() : 0; - return result; + // use the unique ID if it exists + if( null != groupId ){ + return groupId.hashCode(); + } + + return Objects.hash(groupId, clusterId, groupName, serviceName); } /** http://git-wip-us.apache.org/repos/asf/ambari/blob/1584984e/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java index 9668210..7753b63 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/AlertTargetEntity.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.EnumSet; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Set; import javax.persistence.Basic; @@ -384,7 +385,7 @@ public class AlertTargetEntity { } /** - * + * {@inheritDoc} */ @Override public boolean equals(Object object) { @@ -398,21 +399,30 @@ public class AlertTargetEntity { AlertTargetEntity that = (AlertTargetEntity) object; - if (targetId != null ? !targetId.equals(that.targetId) - : that.targetId != null) { - return false; + // use the unique ID if it exists + if( null != targetId ){ + return Objects.equals(targetId, that.targetId); } - return true; - } + return Objects.equals(targetId, that.targetId) && + Objects.equals(targetName, that.targetName) && + Objects.equals(notificationType, that.notificationType) && + Objects.equals(isEnabled, that.isEnabled) && + Objects.equals(description, that.description) && + Objects.equals(isGlobal, that.isGlobal); + } /** - * + * {@inheritDoc} */ @Override public int hashCode() { - int result = null != targetId ? targetId.hashCode() : 0; - return result; + // use the unique ID if it exists + if (null != targetId) { + return targetId.hashCode(); + } + + return Objects.hash(targetId, targetName, notificationType, isEnabled, description, isGlobal); } /** http://git-wip-us.apache.org/repos/asf/ambari/blob/1584984e/ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java b/ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java index 87afb38..ed4a196 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/orm/dao/AlertDispatchDAOTest.java @@ -85,8 +85,6 @@ public class AlertDispatchDAOTest { private AlertDefinitionDAO m_definitionDao; private AlertsDAO m_alertsDao; private OrmTestHelper m_helper; - private HostComponentDesiredStateDAO hostComponentDesiredStateDAO; - private HostComponentStateDAO hostComponentStateDAO; private ServiceFactory m_serviceFactory; private ServiceComponentFactory m_componentFactory; @@ -131,7 +129,7 @@ public class AlertDispatchDAOTest { } private void initTestData() throws Exception { - Set<AlertTargetEntity> targets = createTargets(); + Set<AlertTargetEntity> targets = createTargets(1); for (int i = 0; i < 2; i++) { AlertGroupEntity group = new AlertGroupEntity(); @@ -849,9 +847,9 @@ public class AlertDispatchDAOTest { * @return * @throws Exception */ - private Set<AlertTargetEntity> createTargets() throws Exception { + private Set<AlertTargetEntity> createTargets(int numberOfTargets) throws Exception { Set<AlertTargetEntity> targets = new HashSet<AlertTargetEntity>(); - for (int i = 0; i < 1; i++) { + for (int i = 0; i < numberOfTargets; i++) { AlertTargetEntity target = new AlertTargetEntity(); target.setDescription("Target Description " + i); target.setNotificationType("EMAIL"); @@ -901,4 +899,47 @@ public class AlertDispatchDAOTest { assertTrue(group.getAlertDefinitions().contains(definition)); } } + + /** + * Tests that updating JPA associations concurrently doesn't lead to Concu + * + * @throws Exception + */ + @Test + public void testConcurrentGroupModification() throws Exception { + createDefinitions(); + + AlertGroupEntity group = m_helper.createAlertGroup(m_cluster.getClusterId(), null); + final Set<AlertTargetEntity> targets = createTargets(100); + + group.setAlertTargets(targets); + group = m_dao.merge(group); + + final class AlertGroupWriterThread extends Thread { + private AlertGroupEntity group; + + /** + * {@inheritDoc} + */ + @Override + public void run() { + for (int i = 0; i < 1000; i++) { + group.setAlertTargets(new HashSet<>(targets)); + } + } + } + + List<Thread> threads = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + AlertGroupWriterThread thread = new AlertGroupWriterThread(); + threads.add(thread); + + thread.group = group; + thread.start(); + } + + for (Thread thread : threads) { + thread.join(); + } + } }
