AMBARI-18404 - Upgrade Summary Endpoint Throws NPEs Due To JPA Cached Entities With Missing IDs (jonathanhurley)
Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/da560570 Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/da560570 Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/da560570 Branch: refs/heads/branch-dev-patch-upgrade Commit: da5605706555d3aecca7a67fb399d9c501972ad4 Parents: 1efc99c Author: Jonathan Hurley <jhur...@hortonworks.com> Authored: Thu Sep 15 12:12:06 2016 -0400 Committer: Jonathan Hurley <jhur...@hortonworks.com> Committed: Thu Sep 15 23:57:36 2016 -0400 ---------------------------------------------------------------------- .../actionmanager/ActionDBAccessorImpl.java | 14 +++++----- .../server/actionmanager/HostRoleCommand.java | 16 +++++++++++ .../orm/entities/HostRoleCommandEntity.java | 19 +++++++++++++ .../ambari/server/topology/HostRequest.java | 25 ++++++++--------- .../actionmanager/TestActionDBAccessorImpl.java | 29 ++++++++++++++++++++ 5 files changed, 82 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/da560570/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java b/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java index b7e7f2d..c31ca7e 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java @@ -344,7 +344,6 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { StageEntity stageEntity = stage.constructNewPersistenceEntity(); stageEntities.add(stageEntity); stageEntity.setClusterId(clusterId); - //TODO refactor to reduce merges stageEntity.setRequest(requestEntity); stageDAO.create(stageEntity); @@ -353,9 +352,6 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { for (HostRoleCommand hostRoleCommand : orderedHostRoleCommands) { HostRoleCommandEntity hostRoleCommandEntity = hostRoleCommand.constructNewPersistenceEntity(); hostRoleCommandEntity.setStage(stageEntity); - - HostEntity hostEntity = null; - hostRoleCommandDAO.create(hostRoleCommandEntity); assert hostRoleCommandEntity.getTaskId() != null; @@ -365,6 +361,7 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { String output = "output-" + hostRoleCommandEntity.getTaskId() + ".txt"; String error = "errors-" + hostRoleCommandEntity.getTaskId() + ".txt"; + HostEntity hostEntity = null; if (null != hostRoleCommandEntity.getHostId()) { hostEntity = hostDAO.findById(hostRoleCommandEntity.getHostId()); if (hostEntity == null) { @@ -372,6 +369,7 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { LOG.error(msg); throw new AmbariException(msg); } + hostRoleCommandEntity.setHostEntity(hostEntity); try { @@ -401,9 +399,10 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { hostRoleCommandEntity.setExecutionCommand(executionCommandEntity); executionCommandDAO.create(hostRoleCommandEntity.getExecutionCommand()); - hostRoleCommandDAO.merge(hostRoleCommandEntity); + hostRoleCommandEntity = hostRoleCommandDAO.merge(hostRoleCommandEntity); + if (null != hostEntity) { - hostDAO.merge(hostEntity); + hostEntity = hostDAO.merge(hostEntity); } } @@ -411,8 +410,9 @@ public class ActionDBAccessorImpl implements ActionDBAccessor { roleSuccessCriteriaDAO.create(roleSuccessCriteriaEntity); } - stageDAO.create(stageEntity); + stageEntity = stageDAO.merge(stageEntity); } + requestEntity.setStages(stageEntities); requestDAO.merge(requestEntity); } http://git-wip-us.apache.org/repos/asf/ambari/blob/da560570/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java b/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java index ff2ce92..85c8e9f 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java @@ -209,6 +209,22 @@ public class HostRoleCommand { hostRoleCommandEntity.setEvent(event.getEventJson()); + // set IDs if the wrapping object has them - they are most likely + // non-updatable in JPA since they are retrieved from a relationship, + // however the JPA cache may choose to not refresh the entity so they would + // end up being null if not set before persisting the command entity + if (requestId >= 0) { + hostRoleCommandEntity.setRequestId(requestId); + } + + if (stageId >= 0) { + hostRoleCommandEntity.setStageId(stageId); + } + + if (taskId >= 0) { + hostRoleCommandEntity.setTaskId(taskId); + } + return hostRoleCommandEntity; } http://git-wip-us.apache.org/repos/asf/ambari/blob/da560570/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java index 6288091..74271b9 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java @@ -511,8 +511,27 @@ public class HostRoleCommandEntity { return stage; } + /** + * Sets the associated {@link StageEntity} for this command. If the + * {@link StageEntity} has been persisted, then this will also set the + * commands stage and request ID fields. + * + * @param stage + */ public void setStage(StageEntity stage) { this.stage = stage; + + // ensure that the IDs are also set since they may not be retrieved from JPA + // when this entity is cached + if (null != stage) { + if (null == stageId) { + stageId = stage.getStageId(); + } + + if (null == requestId) { + requestId = stage.getRequestId(); + } + } } public HostEntity getHostEntity() { http://git-wip-us.apache.org/repos/asf/ambari/blob/da560570/ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java b/ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java index c3b44b8..6a65b48 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java @@ -18,6 +18,17 @@ package org.apache.ambari.server.topology; +import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_AND_START; +import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_ONLY; +import static org.apache.ambari.server.controller.internal.ProvisionAction.START_ONLY; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; + import org.apache.ambari.server.actionmanager.HostRoleCommand; import org.apache.ambari.server.api.predicate.InvalidQueryException; import org.apache.ambari.server.api.predicate.PredicateCompiler; @@ -37,17 +48,6 @@ import org.apache.ambari.server.state.host.HostImpl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.ArrayList; -import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; - -import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_AND_START; -import static org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_ONLY; -import static org.apache.ambari.server.controller.internal.ProvisionAction.START_ONLY; - /** * Represents a set of requests to a single host such as install, start, etc. */ @@ -355,9 +355,6 @@ public class HostRequest implements Comparable<HostRequest> { for (HostRoleCommand task : logicalTasks.values()) { HostRoleCommandEntity entity = task.constructNewPersistenceEntity(); // the above method doesn't set all of the fields for some unknown reason - entity.setRequestId(task.getRequestId()); - entity.setStageId(task.getStageId()); - entity.setTaskId(task.getTaskId()); entity.setOutputLog(task.getOutputLog()); entity.setErrorLog(task.errorLog); http://git-wip-us.apache.org/repos/asf/ambari/blob/da560570/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java b/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java index 0813dff..bf9d0db 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java @@ -27,6 +27,7 @@ import java.util.Collections; import java.util.List; import javax.persistence.EntityManager; +import javax.persistence.NamedQuery; import org.apache.ambari.server.AmbariException; import org.apache.ambari.server.Role; @@ -611,6 +612,34 @@ public class TestActionDBAccessorImpl { } + /** + * Tests that entities created int he {@link ActionDBAccessor} can be + * retrieved with their IDs intact. EclipseLink seems to execute the + * {@link NamedQuery} but then use its cached entities to fill in the data. + * + * @throws Exception + */ + @Test + public void testEntitiesCreatedWithIDs() throws Exception { + List<Stage> stages = new ArrayList<Stage>(); + Stage stage = createStubStage(hostName, requestId, stageId); + + stages.add(stage); + + Request request = new Request(stages, clusters); + + // persist entities + db.persistActions(request); + + // query entities immediately to ensure IDs are populated + List<HostRoleCommandEntity> commandEntities = hostRoleCommandDAO.findByRequest(requestId); + Assert.assertEquals(2, commandEntities.size()); + + for (HostRoleCommandEntity entity : commandEntities) { + Assert.assertEquals(Long.valueOf(requestId), entity.getRequestId()); + Assert.assertEquals(Long.valueOf(stageId), entity.getStageId()); + } + } private static class TestActionDBAccessorModule extends AbstractModule { @Override