AMBARI-17738 - EclipseLink Sequence Query Stuck Inside of Transaction And Blocks Other Threads (jonathanhurley)

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java

@@ -222,7 +222,7 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
   private static final Map<Resource.Type, String> KEY_PROPERTY_IDS = new HashMap<>();
 
   @Inject
-  private static UpgradeDAO s_upgradeDAO = null;
+  protected static UpgradeDAO s_upgradeDAO = null;
 
   @Inject
   private static Provider<AmbariMetaInfo> s_metaProvider = null;
@@ -691,7 +691,6 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
       List<Resource> failedResources = new LinkedList<Resource>();
       if (preUpgradeCheckResources != null) {
         for (Resource res : preUpgradeCheckResources) {
-          String id = (String) res.getPropertyValue((PreUpgradeCheckResourceProvider.UPGRADE_CHECK_ID_PROPERTY_ID));
           PrereqCheckStatus prereqCheckStatus = (PrereqCheckStatus) res.getPropertyValue(
               PreUpgradeCheckResourceProvider.UPGRADE_CHECK_STATUS_PROPERTY_ID);
 
@@ -747,7 +746,24 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
     }
   }
 
-  @Transactional
+  /**
+   * Creates the upgrade. All Request/Stage/Task and Upgrade entities will exist
+   * in the database when this method completes.
+   * <p/>
+   * This method itself must not be wrapped in a transaction since it can
+   * potentially make 1000's of database calls while building the entities
+   * before persisting them. This would create a long-lived transaction which
+   * could lead to database deadlock issues. Instead, only the creation of the
+   * actual entities is wrapped in a {@link Transactional} block.
+   *
+   * @param cluster
+   * @param direction
+   * @param pack
+   * @param requestMap
+   * @return
+   * @throws AmbariException
+   * @throws AuthorizationException
+   */
   protected UpgradeEntity createUpgrade(Cluster cluster, Direction direction, UpgradePack pack,
     Map<String, Object> requestMap) throws AmbariException, AuthorizationException  {
 
@@ -978,15 +994,40 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
     entity.setDowngradeAllowed(pack.isDowngradeAllowed());
 
     req.getRequestStatusResponse();
+    return createUpgradeInsideTransaction(cluster, req, entity);
+  }
+
+  /**
+   * Creates the Request/Stage/Task entities and the Upgrade entities inside of
+   * a single transaction. We break this out since the work to get us to this
+   * point could take a very long time and involve many queries to the database
+   * as the commands are being built.
+   *
+   * @param cluster
+   *          the cluster (not {@code null}).
+   * @param request
+   *          the request to persist with all stages and tasks created in memory
+   *          (not {@code null}).
+   * @param upgradeEntity
+   *          the upgrade to create and associate with the newly created request
+   *          (not {@code null}).
+   * @return the persisted {@link UpgradeEntity} encapsulating all
+   *         {@link UpgradeGroupEntity} and {@link UpgradeItemEntity}.
+   * @throws AmbariException
+   */
+  @Transactional
+  private UpgradeEntity createUpgradeInsideTransaction(Cluster cluster,
+      RequestStageContainer request,
+      UpgradeEntity upgradeEntity) throws AmbariException {
 
-    entity.setRequestId(req.getId());
+    upgradeEntity.setRequestId(request.getId());
 
-    req.persist();
+    request.persist();
 
-    s_upgradeDAO.create(entity);
-    cluster.setUpgradeEntity(entity);
+    s_upgradeDAO.create(upgradeEntity);
+    cluster.setUpgradeEntity(upgradeEntity);
 
-    return entity;
+    return upgradeEntity;
   }
 
   /**
@@ -1544,7 +1585,7 @@ public class UpgradeResourceProvider extends AbstractControllerResourceProvider
     switch (task.getType()) {
       case SERVER_ACTION:
       case MANUAL: {
-        ServerSideActionTask serverTask = (ServerSideActionTask) task;
+        ServerSideActionTask serverTask = task;
 
         if (null != serverTask.summary) {
           stageText = serverTask.summary;

ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeEntity.java

@@ -44,7 +44,8 @@ import org.apache.ambari.server.state.stack.upgrade.UpgradeType;
 @Table(name = "upgrade")
 @TableGenerator(name = "upgrade_id_generator",
     table = "ambari_sequences", pkColumnName = "sequence_name", valueColumnName = "sequence_value",
-    pkColumnValue = "upgrade_id_seq", initialValue = 0)
+    pkColumnValue = "upgrade_id_seq",
+    initialValue = 0)
 @NamedQueries({
   @NamedQuery(name = "UpgradeEntity.findAll",
       query = "SELECT u FROM UpgradeEntity u"),

ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeGroupEntity.java

@@ -33,13 +33,20 @@ import javax.persistence.Table;
 import javax.persistence.TableGenerator;
 
 /**
- * Models a single upgrade group as part of an entire {@link UpgradeEntity}
+ * Models a single upgrade group as part of an entire {@link UpgradeEntity}.
+ * <p/>
+ * Since {@link UpgradeGroupEntity} instances are rarely created, yet created in
+ * bulk, we have an abnormally high {@code allocationSize}} for the
+ * {@link TableGenerator}. This helps prevent locks caused by frequenty queries
+ * to the sequence ID table.
  */
 @Table(name = "upgrade_group")
 @Entity
 @TableGenerator(name = "upgrade_group_id_generator",
     table = "ambari_sequences", pkColumnName = "sequence_name", valueColumnName = "sequence_value",
-    pkColumnValue = "upgrade_group_id_seq", initialValue = 0)
+    pkColumnValue = "upgrade_group_id_seq",
+    initialValue = 0,
+    allocationSize = 200)
 public class UpgradeGroupEntity {
 
   @Id

ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeItemEntity.java

@@ -30,16 +30,24 @@ import javax.persistence.ManyToOne;
 import javax.persistence.Table;
 import javax.persistence.TableGenerator;
 
+import org.apache.ambari.server.actionmanager.Stage;
 import org.apache.ambari.server.state.UpgradeState;
 
 /**
- * Models a single upgrade item as part of
+ * Models a single upgrade item which is directly associated with {@link Stage}.
+ * <p/>
+ * Since {@link UpgradeItemEntity} instances are rarely created, yet created in
+ * bulk, we have an abnormally high {@code allocationSize}} for the
+ * {@link TableGenerator}. This helps prevent locks caused by frequenty queries
+ * to the sequence ID table.
  */
 @Table(name = "upgrade_item")
 @Entity
 @TableGenerator(name = "upgrade_item_id_generator",
     table = "ambari_sequences", pkColumnName = "sequence_name", valueColumnName = "sequence_value",
-    pkColumnValue = "upgrade_item_id_seq", initialValue = 0)
+    pkColumnValue = "upgrade_item_id_seq",
+    initialValue = 0,
+    allocationSize = 1000)
 public class UpgradeItemEntity {
 
   @Id

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java

@@ -17,11 +17,11 @@
  */
 package org.apache.ambari.server.controller.internal;
 
-import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.anyLong;
 import static org.easymock.EasyMock.createNiceMock;
-import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.eq;
-import static org.easymock.EasyMock.anyLong;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -104,8 +104,12 @@ import org.easymock.EasyMock;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.api.easymock.PowerMock;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
 
 import com.google.gson.Gson;
 import com.google.gson.JsonArray;
@@ -117,11 +121,6 @@ import com.google.inject.Injector;
 import com.google.inject.Module;
 import com.google.inject.persist.PersistService;
 import com.google.inject.util.Modules;
-import org.junit.runner.RunWith;
-import org.powermock.api.easymock.PowerMock;
-import org.powermock.core.classloader.annotations.PowerMockIgnore;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
 
 /**
  * UpgradeResourceDefinition tests.
@@ -1031,7 +1030,6 @@ public class UpgradeResourceProviderTest {
 
 
   @Test
-  @Ignore
   public void testCreateCrossStackUpgrade() throws Exception {
     Cluster cluster = clusters.getCluster("c1");
     StackId oldStack = cluster.getDesiredStackVersion();
@@ -1062,7 +1060,7 @@ public class UpgradeResourceProviderTest {
     Map<String, Object> requestProps = new HashMap<String, Object>();
     requestProps.put(UpgradeResourceProvider.UPGRADE_CLUSTER_NAME, "c1");
     requestProps.put(UpgradeResourceProvider.UPGRADE_VERSION, "2.2.0.0");
-    requestProps.put(UpgradeResourceProvider.UPGRADE_PACK, "upgrade_test_nonrolling");
+    requestProps.put(UpgradeResourceProvider.UPGRADE_PACK, "upgrade_test");
     requestProps.put(UpgradeResourceProvider.UPGRADE_SKIP_PREREQUISITE_CHECKS, "true");
 
     ResourceProvider upgradeResourceProvider = createProvider(amc);
@@ -1074,13 +1072,13 @@ public class UpgradeResourceProviderTest {
     assertEquals(1, upgrades.size());
 
     UpgradeEntity upgrade = upgrades.get(0);
-    assertEquals(5, upgrade.getUpgradeGroups().size());
+    assertEquals(3, upgrade.getUpgradeGroups().size());
 
     UpgradeGroupEntity group = upgrade.getUpgradeGroups().get(2);
-    assertEquals(1, group.getItems().size());
+    assertEquals(2, group.getItems().size());
 
     group = upgrade.getUpgradeGroups().get(0);
-    assertEquals(1, group.getItems().size());
+    assertEquals(2, group.getItems().size());
 
     assertTrue(cluster.getDesiredConfigs().containsKey("zoo.cfg"));
 
@@ -1393,6 +1391,44 @@ public class UpgradeResourceProviderTest {
     }
   }
 
+  /**
+   * Tests that an error while commiting the data cleanly rolls back the transaction so that
+   * no request/stage/tasks are created.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testRollback() throws Exception {
+    Cluster cluster = clusters.getCluster("c1");
+
+    Map<String, Object> requestProps = new HashMap<String, Object>();
+    requestProps.put(UpgradeResourceProvider.UPGRADE_CLUSTER_NAME, "c1");
+    requestProps.put(UpgradeResourceProvider.UPGRADE_VERSION, "2.2.0.0");
+    requestProps.put(UpgradeResourceProvider.UPGRADE_PACK, "upgrade_test");
+    requestProps.put(UpgradeResourceProvider.UPGRADE_TYPE, UpgradeType.ROLLING.toString());
+    requestProps.put(UpgradeResourceProvider.UPGRADE_SKIP_MANUAL_VERIFICATION, Boolean.FALSE.toString());
+    requestProps.put(UpgradeResourceProvider.UPGRADE_SKIP_PREREQUISITE_CHECKS, Boolean.TRUE.toString());
+
+    // this will cause a NPE when creating the upgrade, allowing us to test
+    // rollback
+    UpgradeResourceProvider upgradeResourceProvider = createProvider(amc);
+    upgradeResourceProvider.s_upgradeDAO = null;
+
+    try {
+      Request request = PropertyHelper.getCreateRequest(Collections.singleton(requestProps), null);
+      upgradeResourceProvider.createResources(request);
+      Assert.fail("Expected a NullPointerException");
+    } catch (NullPointerException npe) {
+      // expected
+    }
+
+    List<UpgradeEntity> upgrades = upgradeDao.findUpgrades(cluster.getClusterId());
+    assertEquals(0, upgrades.size());
+
+    List<Long> requestIds = requestDao.findAllRequestIds(1, true, cluster.getClusterId());
+    assertEquals(0, requestIds.size());
+  }
+
   private String parseSingleMessage(String msgStr){
     JsonParser parser = new JsonParser();
     JsonArray msgArray = (JsonArray) parser.parse(msgStr);