Repository: ambari Updated Branches: refs/heads/trunk a18aa6700 -> 93f5798e0
AMBARI-17738 - EclipseLink Sequence Query Stuck Inside of Transaction And Blocks Other Threads (jonathanhurley) Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/93f5798e Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/93f5798e Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/93f5798e Branch: refs/heads/trunk Commit: 93f5798e09e2065ebd27552eb8f2e88fb95e0390 Parents: a18aa67 Author: Jonathan Hurley <[email protected]> Authored: Fri Jul 15 11:52:27 2016 -0400 Committer: Jonathan Hurley <[email protected]> Committed: Fri Jul 15 20:16:20 2016 -0400 ---------------------------------------------------------------------- .../internal/UpgradeResourceProvider.java | 59 +++++++++++++++--- .../server/orm/entities/UpgradeEntity.java | 3 +- .../server/orm/entities/UpgradeGroupEntity.java | 11 +++- .../server/orm/entities/UpgradeItemEntity.java | 12 +++- .../internal/UpgradeResourceProviderTest.java | 64 +++++++++++++++----- 5 files changed, 121 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/93f5798e/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java b/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java index 2e976ba..c390f44 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java +++ b/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; http://git-wip-us.apache.org/repos/asf/ambari/blob/93f5798e/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeEntity.java index db27ea5..2c5cbfb 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeEntity.java +++ b/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"), http://git-wip-us.apache.org/repos/asf/ambari/blob/93f5798e/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeGroupEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeGroupEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeGroupEntity.java index 96f96d5..4830e3b 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeGroupEntity.java +++ b/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 http://git-wip-us.apache.org/repos/asf/ambari/blob/93f5798e/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeItemEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeItemEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeItemEntity.java index 6e4a889..560970a 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UpgradeItemEntity.java +++ b/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 http://git-wip-us.apache.org/repos/asf/ambari/blob/93f5798e/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java b/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java index a5db0f0..63892cf 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java +++ b/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);
