[1120417] Break up transaction of updatePluginConfigurationDefinition…

… into smaller pieces Another pass here given some oracle test failures in the CI env. - Fix an issue with PropertyDefinitionSimple.removeEnumeratedValues. An unexpected problem brought out, I guess, by the Tx reworking, must be careful not to replace hibernate proxy dealing with orphanRemoval. - remove unnecessary REQUIRES_NEW that could lead to locking issues - remove some dead code - start shortening xxxInNewTransaction to xxxInNewTx, purely for selfish reasons.
rhq-project · Jul 25, 2014 · 001a3d1 · 001a3d1
1 parent fb8d001
commit 001a3d1
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 40 deletions.
diff --git a/...main/java/org/rhq/core/domain/configuration/definition/PropertyDefinitionEnumeration.java b/...main/java/org/rhq/core/domain/configuration/definition/PropertyDefinitionEnumeration.java
@@ -77,14 +77,6 @@ public PropertyDefinitionEnumeration(@NotNull String name, String value) {
         this.value = value;
     }
 
-    //    @PrePersist
-    //    @PreUpdate
-    //    public void updateOrder() {
-    //        if (this.orderIndex < 0) {
-    //            this.orderIndex = this.propertyDefinitionSimple.getEnumeratedValues().indexOf(this);
-    //        }
-    //    }
-
     public PropertyDefinitionSimple getPropertyDefinitionSimple() {
         return propertyDefinitionSimple;
     }

diff --git a/.../src/main/java/org/rhq/core/domain/configuration/definition/PropertyDefinitionSimple.java b/.../src/main/java/org/rhq/core/domain/configuration/definition/PropertyDefinitionSimple.java
@@ -40,7 +40,6 @@
 import javax.xml.bind.annotation.XmlAccessorType;
 import javax.xml.bind.annotation.XmlRootElement;
 
-import org.hibernate.annotations.Cascade;
 import org.hibernate.annotations.IndexColumn;
 import org.jetbrains.annotations.NotNull;
 
@@ -68,16 +67,14 @@ public class PropertyDefinitionSimple extends PropertyDefinition {
     @Column(name = "ALLOW_CUSTOM_ENUM_VALUE")
     private boolean allowCustomEnumeratedValue = false;
 
-    @Cascade( { org.hibernate.annotations.CascadeType.ALL, org.hibernate.annotations.CascadeType.DELETE_ORPHAN })
-    @OneToMany(mappedBy = "propertyDefinitionSimple", cascade = { CascadeType.ALL }, fetch = FetchType.EAGER)
+    @OneToMany(mappedBy = "propertyDefinitionSimple", cascade = { CascadeType.ALL }, fetch = FetchType.EAGER, orphanRemoval = true)
     private Set<Constraint> constraints = null;
 
     /**
      * The <options> within <property-options> for a <simple-property>
      */
-    @Cascade( { org.hibernate.annotations.CascadeType.ALL, org.hibernate.annotations.CascadeType.DELETE_ORPHAN })
     @IndexColumn(name = "order_index")
-    @OneToMany(mappedBy = "propertyDefinitionSimple", fetch = FetchType.EAGER)
+    @OneToMany(mappedBy = "propertyDefinitionSimple", fetch = FetchType.EAGER, cascade = CascadeType.ALL, orphanRemoval = true)
     private List<PropertyDefinitionEnumeration> enumeratedValues = null;
 
     /**
@@ -98,12 +95,12 @@ public class PropertyDefinitionSimple extends PropertyDefinition {
     @Enumerated(EnumType.ORDINAL)
     private MeasurementUnits units;
 
-    @Cascade( { org.hibernate.annotations.CascadeType.ALL, org.hibernate.annotations.CascadeType.DELETE_ORPHAN })
-    @OneToMany(mappedBy = "propertyDefinition", fetch = FetchType.EAGER)
+    @OneToMany(mappedBy = "propertyDefinition", fetch = FetchType.EAGER, cascade = CascadeType.ALL, orphanRemoval = true)
     List<PropertyOptionsSource> optionsSource = null;
 
-    public PropertyDefinitionSimple(@NotNull String name, String description, boolean required,
-        @NotNull PropertySimpleType type) {
+    public PropertyDefinitionSimple(@NotNull
+    String name, String description, boolean required, @NotNull
+    PropertySimpleType type) {
         super(name, description, required);
         this.type = type;
     }
@@ -127,28 +124,34 @@ public void setType(PropertySimpleType type) {
      */
     @NotNull
     public Set<Constraint> getConstraints() {
-        if (this.constraints==null) {
+        if (this.constraints == null) {
             return Collections.EMPTY_SET;
         }
         return this.constraints;
     }
 
     public void setConstraints(Set<Constraint> constraints) {
-        if (constraints==null || constraints.isEmpty()) {
-            this.constraints=null;
+        // Don't replace the possible Hibernate proxy when orphanRemoval=true. It can cause
+        // "collection with cascade=all-delete-orphan was no longer referenced" exceptions.
+        if (constraints == null || constraints.isEmpty()) {
+            if (null != this.constraints) {
+                this.constraints.clear();
+            }
             return;
         }
-        if (this.constraints==null) {
+
+        if (this.constraints == null) {
             this.constraints = new HashSet<Constraint>(constraints.size());
         }
+
         for (Constraint constraint : constraints) {
             getConstraints().add(constraint);
             constraint.setPropertyDefinitionSimple(this);
         }
     }
 
     public void addConstraints(Constraint... constraintsToAdd) {
-        if (this.constraints==null) {
+        if (this.constraints == null) {
             this.constraints = new HashSet<Constraint>(constraintsToAdd.length);
         }
         for (Constraint constraint : constraintsToAdd) {
@@ -163,7 +166,7 @@ public void addConstraints(Constraint... constraintsToAdd) {
      */
     @NotNull
     public List<PropertyDefinitionEnumeration> getEnumeratedValues() {
-        if (this.enumeratedValues==null) {
+        if (this.enumeratedValues == null) {
             return new ArrayList<PropertyDefinitionEnumeration>(1);
         }
         return this.enumeratedValues;
@@ -176,7 +179,7 @@ public void setEnumeratedValues(List<PropertyDefinitionEnumeration> enumeratedVa
     }
 
     public void addEnumeratedValues(PropertyDefinitionEnumeration... enumerations) {
-        if (this.enumeratedValues==null) {
+        if (this.enumeratedValues == null) {
             this.enumeratedValues = new ArrayList<PropertyDefinitionEnumeration>(1);
         }
         for (PropertyDefinitionEnumeration enumeration : enumerations) {
@@ -187,12 +190,11 @@ public void addEnumeratedValues(PropertyDefinitionEnumeration... enumerations) {
     }
 
     public void removeEnumeratedValues(PropertyDefinitionEnumeration... enumerations) {
+        // Don't replace the possible Hibernate proxy when orphanRemoval=true. It can cause
+        // "collection with cascade=all-delete-orphan was no longer referenced" exceptions.
         for (PropertyDefinitionEnumeration enumeration : enumerations) {
             getEnumeratedValues().remove(enumeration);
         }
-        if (this.enumeratedValues.isEmpty()) {
-            this.enumeratedValues=null;
-        }
         ensureOrdering();
     }
 
@@ -266,17 +268,17 @@ public void setUnits(MeasurementUnits units) {
     }
 
     public PropertyOptionsSource getOptionsSource() {
-        if (optionsSource==null || optionsSource.isEmpty())
+        if (optionsSource == null || optionsSource.isEmpty())
             return null;
         return optionsSource.get(0);
     }
 
     public void setOptionsSource(PropertyOptionsSource source) {
-        if (this.optionsSource==null) {
+        if (this.optionsSource == null) {
             optionsSource = new ArrayList<PropertyOptionsSource>(1);
         }
         this.optionsSource.clear();
-        if (source==null)
+        if (source == null)
             return;
         source.propertyDefinition = this;
         this.optionsSource.add(source);

diff --git a/...va/org/rhq/enterprise/server/configuration/metadata/ConfigurationMetadataManagerBean.java b/...va/org/rhq/enterprise/server/configuration/metadata/ConfigurationMetadataManagerBean.java
@@ -278,6 +278,7 @@ private void updateTemplate(ConfigurationTemplate existingDT, ConfigurationTempl
                     }
                 }
             }
+
             entityManager.flush();
         } catch (Throwable t) {
             t.printStackTrace();
@@ -340,7 +341,7 @@ private void updatePropertyDefinition(PropertyDefinition existingProperty, Prope
         if (existingProperty instanceof PropertyDefinitionMap) {
             if (newProperty instanceof PropertyDefinitionMap) {
 
-                // alter existingPropDefs to reflect newPropDefs                
+                // alter existingPropDefs to reflect newPropDefs
                 Map<String, PropertyDefinition> existingPropDefs = ((PropertyDefinitionMap) existingProperty)
                     .getMap();
 
@@ -432,7 +433,6 @@ private void updatePropertyDefinition(PropertyDefinition existingProperty, Prope
 
                 // handle <constraint> [0..*]
 
-                //                entityManager.flush();
                 Set<Constraint> exCon = existingPDS.getConstraints();
                 if (exCon.size() > 0) {
                     for (Constraint con : exCon) {
@@ -474,7 +474,6 @@ private void replaceProperty(PropertyDefinition existingProperty, PropertyDefini
 
         entityManager.remove(existingProperty);
         entityManager.merge(configDef);
-        //        entityManager.flush();
     }
 
     /**
@@ -518,7 +517,6 @@ private void replaceListProperty(PropertyDefinitionList exList, PropertyDefiniti
         }
 
         entityManager.merge(exList);
-        //        entityManager.flush();
     }
 
     /**

diff --git a/...org/rhq/enterprise/server/resource/metadata/ResourceConfigurationMetadataManagerBean.java b/...org/rhq/enterprise/server/resource/metadata/ResourceConfigurationMetadataManagerBean.java
@@ -2,8 +2,6 @@
 
 import javax.ejb.EJB;
 import javax.ejb.Stateless;
-import javax.ejb.TransactionAttribute;
-import javax.ejb.TransactionAttributeType;
 import javax.persistence.EntityManager;
 import javax.persistence.PersistenceContext;
 
@@ -27,7 +25,6 @@ public class ResourceConfigurationMetadataManagerBean implements ResourceConfigu
     private ConfigurationMetadataManagerLocal configurationMetadataMgr;
 
     @Override
-    @TransactionAttribute(TransactionAttributeType.REQUIRES_NEW)
     public void updateResourceConfigurationDefinition(ResourceType existingType, ResourceType newType) {
         log.debug("Updating resource configuration definition for " + existingType);
 

diff --git a/...rc/main/java/org/rhq/enterprise/server/resource/metadata/ResourceMetadataManagerBean.java b/...rc/main/java/org/rhq/enterprise/server/resource/metadata/ResourceMetadataManagerBean.java
@@ -161,7 +161,7 @@ public void updateTypes(Set<ResourceType> resourceTypes) throws Exception {
         Set<ResourceType> legitimateChildren = new HashSet<ResourceType>();
         for (ResourceType resourceType : nonRunsInsideResourceTypes) {
             long startTime = System.currentTimeMillis();
-            resourceType = resourceMetadataManager.updateTypeInNewTransaction(resourceType);
+            resourceType = resourceMetadataManager.updateTypeInNewTx(resourceType);
             long endTime = System.currentTimeMillis();
             log.debug("Updated resource type [" + toConciseString(resourceType) + "] in " + (endTime - startTime)
                 + " ms");
@@ -314,7 +314,7 @@ private void removeFromChildren(ResourceType typeToBeRemoved) {
 
     @TransactionTimeout(1800)
     @TransactionAttribute(TransactionAttributeType.REQUIRES_NEW)
-    public ResourceType updateTypeInNewTransaction(ResourceType resourceType) {
+    public ResourceType updateTypeInNewTx(ResourceType resourceType) {
 
         // see if there is already an existing type that we need to update
         log.info("Updating resource type [" + toConciseString(resourceType) + "]...");

diff --git a/...c/main/java/org/rhq/enterprise/server/resource/metadata/ResourceMetadataManagerLocal.java b/...c/main/java/org/rhq/enterprise/server/resource/metadata/ResourceMetadataManagerLocal.java
@@ -52,7 +52,7 @@ void getPluginTypes(Subject subject, String pluginName, Set<ResourceType> legitT
      * <p/>
      * Has a 30 minute Tx timeout to allow for large updates.
      */
-    ResourceType updateTypeInNewTransaction(ResourceType resourceType) throws Exception;
+    ResourceType updateTypeInNewTx(ResourceType resourceType) throws Exception;
 
     /** TODO: do we want to create a driftMetadataManager SLSB and put this in there */
     void updateDriftMetadata(ResourceType existingType, ResourceType resourceType);