You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ambari.apache.org by jo...@apache.org on 2016/12/20 21:52:06 UTC

ambari git commit: AMBARI-19259 - When Updating An Alert Group a ConcurrentModificationException is Thrown (jonathanhurley)

Repository: ambari
Updated Branches:
  refs/heads/trunk 4fc52a61f -> 1584984e8


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/trunk
Commit: 1584984e8836f6951794b3ac0309494cc9acb234
Parents: 4fc52a6
Author: Jonathan Hurley <jh...@hortonworks.com>
Authored: Tue Dec 20 15:17:28 2016 -0500
Committer: Jonathan Hurley <jh...@hortonworks.com>
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();
+    }
+  }
 }