AMBARI-23130 Persist mpack information instead of the full provision request #603

benyoka · 2018-03-09T12:24:11Z

What changes were proposed in this pull request?

This a followup for #559.

In a previous PR I implemented persisting the raw request in the topology_request entity. After a discussion with @rnettleton it turned out this is not the ideal way. A much desirable way is to persist the raw provision request as a cluster artifact so that it will be retrievable via REST API (with passwords filtered out). This effort will be tracked in a separate JIRA.

This PR modifies the original PR by persisting only the mpack related information in the TopologyRequestEntity. This information is still needed to replay persisted topology requests.

How was this patch tested?

Tested manually
Wrote new unit tests
Unit test results:
Tests run: 4960, Failures: 31, Errors: 93, Skipped: 43

…ids on server restart (benyoka)

…eature-AMBARI-14714

benyoka · 2018-03-09T12:26:23Z

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

  public void setProvisionAction(ProvisionAction provisionAction) {
    this.provisionAction = provisionAction;
  }
+


This is moved here from PersistedStateImpl to take advantage of polymorphism instead of doing instanceof checks.

jonathan-hurley · 2018-03-09T14:40:56Z

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

+    return entity;
+  }
+
+  private TopologyHostGroupEntity toHostGroupEntity(HostGroupInfo groupInfo, TopologyRequestEntity topologyRequestEntity) {


jonathan-hurley · 2018-03-09T14:41:58Z

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

+      for (String hostName : hosts) {
+        TopologyHostInfoEntity hostInfoEntity = new TopologyHostInfoEntity();
+        hostInfoEntity.setTopologyHostGroupEntity(entity);
+        if (groupInfo.getPredicate() != null) {


Is the if-check necessary? Can you just set it to the value of the predicate, even if the predicate is null?

I actually didn't write this method but copied it over from PersistedStateImpl. I can change it nevertheless.

jonathan-hurley · 2018-03-09T14:42:25Z

ambari-server/src/main/java/org/apache/ambari/server/utils/JsonUtils.java

    }
  }

+  public static <T> T fromJson(String json, Class<?> valueType) {


jonathan-hurley · 2018-03-09T14:43:25Z

ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql

  cluster_id BIGINT NOT NULL,
  bp_name VARCHAR(100) NOT NULL,
-  raw_request_body CLOB NOT NULL,
+  mpack_instances CLOB NOT NULL,


Can you give an example of what mpack_instances looks like? Does it make sense to try to normalize this data into its own table?

It has the same structure as for blueprints. For blueprints it is modelled by the BlueprintMpackInstanceEntity, BlueprintServiceEntity, BlueprintMpackConfigEntities.

Currently we are interested in the stack id's only, later also in configurations and service descriptors associated with mpacks (I saw configs are already dumped as json too).

Do you think it is worth normalizing? (would mean 3 more tables, later maybe more) This information wouldn't be used outside of the scope of the replayed request.

My concern is always that when de-serializing this, if the object it's being mapped to changed in any way, the deserialization will fail? We've actually tried to move away from this model. Repositories used to be a solid chunk of JSON but we recently broke this out into 3 new tables to better manage it as entities.

I hate giant blob of JSON, especially if we need to worry about deserializing it ourselves and pulling data out of it.

Based on @jonathan-hurley 's feedback, and also based on looking at the BlueprintMPackInstanceEntity, it seems like this data should be normalized.

Since the MPack entities can be defined in either the Blueprint or the cluster creation template, it seems like it might make sense to use the same tables to store them.

Is there some technical reason for storing this data in the topology_request table? It seems to me that we'd be better off persisting all MPack instance data in the same table, regardless of whether it is defined in the Blueprint or the Cluster Creation template.

Since Service instance definitions and config can be specified in either document, I'm not sure why these should be modelled separately based on the location of the definition. It looks like we should be using the existing tables for both cases.

On the other hand, this shouldn't be a problem for blueprints, since backward compatibility is required. If users should be able to successfully submit "old" blueprint / cluster creation requests even if code is changed, so should Ambari internally.

jonathan-hurley

Some minor changes and a question.

adoroszlai · 2018-03-09T14:32:37Z

ambari-server/src/main/java/org/apache/ambari/server/utils/JsonUtils.java

  private static final ObjectMapper JSON_SERIALIZER = new ObjectMapper();
+  static {
+    JSON_SERIALIZER.setSerializationInclusion(JsonInclude.Include.NON_NULL);
+  }


setSerializationInclusion and other similar methods return the mapper, so it can be chained like:

private static final ObjectMapper JSON_SERIALIZER = new ObjectMapper() .setSerializationInclusion(JsonInclude.Include.NON_NULL);

rnettleton

The code changes in the patch look fine to me, but it looks like it may be worth considering using the same database tables to store the mpack instance information, regardless of whether this information is stored in a Blueprint or a Cluster Creation template.

rnettleton · 2018-03-09T16:05:15Z

ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql

  cluster_id BIGINT NOT NULL,
  bp_name VARCHAR(100) NOT NULL,
-  raw_request_body CLOB NOT NULL,
+  mpack_instances CLOB NOT NULL,


Based on @jonathan-hurley 's feedback, and also based on looking at the BlueprintMPackInstanceEntity, it seems like this data should be normalized.

Since the MPack entities can be defined in either the Blueprint or the cluster creation template, it seems like it might make sense to use the same tables to store them.

Is there some technical reason for storing this data in the topology_request table? It seems to me that we'd be better off persisting all MPack instance data in the same table, regardless of whether it is defined in the Blueprint or the Cluster Creation template.

Since Service instance definitions and config can be specified in either document, I'm not sure why these should be modelled separately based on the location of the definition. It looks like we should be using the existing tables for both cases.

asfgit · 2018-03-09T19:46:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1067/
Test FAILed.
Test FAILured.

asfgit · 2018-03-09T20:35:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1068/
Test FAILed.
Test FAILured.

asfgit · 2018-03-10T02:01:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1074/
Test FAILed.
Test FAILured.

benyoka · 2018-03-13T11:18:01Z

I implemented normalized persistence for topology requests mpack information. Tables are shared with blueprint mpack information using JPA's single table inheritance. Tables and entities have been renamed to reflect the decoupling from blueprints (mpack instance entities can belong to topology requests too).
@rnettleton @jonathan-hurley @adoroszlai

asfgit · 2018-03-13T12:06:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1127/
Test FAILed.
Test FAILured.

…eature-AMBARI-14714

rnettleton · 2018-03-14T15:00:59Z

I think the patch looks fine to me, but we should ask @jonathan-hurley to re-review, to make sure the table design and persistence code is correct.

jonathan-hurley · 2018-03-14T15:18:53Z

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

+  private String mpackUri;
+
+  @OneToMany(cascade = CascadeType.ALL, mappedBy = "mpackInstance")
+  private Collection<MpackInstanceServiceEntity> serviceInstances = new ArrayList<>();


Why make these a Collection instead of a List?

I prefer using the most general suitable interface.

jonathan-hurley · 2018-03-14T15:24:52Z

ambari-server/src/main/java/org/apache/ambari/server/topology/MpackInstance.java

+    return mpackInstanceEntity;
+  }
+
+  private void setCommonProperties(MpackInstanceEntity mpackInstanceEntity) {


Documentation of which properties are being set for the bi-directional relationship.

jonathan-hurley · 2018-03-14T15:26:15Z

ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql

  CONSTRAINT PK_hosts PRIMARY KEY (host_id),
  CONSTRAINT UQ_hosts_host_name UNIQUE (host_name));

+CREATE TABLE mpack_host_state (


Any reason you moved this?

To have the tables in the same order as in Ambari-DDL-Postgres-CREATE.sql for easier comparision.

jonathan-hurley

Some doc/minor changes.

Any reason you didn't use orphanRemoval on the collections?

benyoka · 2018-03-19T09:08:53Z

Hey @jonathan-hurley

Re orphanRemoval: these entities are saved once and are not expected to be updated later. I can add orphanRemoval though. I noticed that orphanRemoval is missing in other places where updates are not expected.

asfgit · 2018-03-19T11:19:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/1227/
Test FAILed.
Test FAILured.

Balazs Bence Sari added 6 commits March 6, 2018 16:00

AMBARI-23130 persist raw cluster provision request and extract stack …

34cda14

…ids on server restart (benyoka)

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

9902c61

…eature-AMBARI-14714

AMBARI-23130 add columnt to other DDLs + fix DDLs (benyoka)

2e8c8bf

AMBARI-23130 fix review findings (benyoka)

386c582

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

210f390

…eature-AMBARI-14714

AMBARI-23130 persist only mpack instances instead of the full request

fd24060

benyoka requested review from adoroszlai, jonathan-hurley and rnettleton March 9, 2018 12:24

benyoka changed the base branch from trunk to branch-feature-AMBARI-14714 March 9, 2018 12:25

benyoka commented Mar 9, 2018

View reviewed changes

jonathan-hurley reviewed Mar 9, 2018

View reviewed changes

adoroszlai approved these changes Mar 9, 2018

View reviewed changes

AMBARI-23130 address review findings (benyoka)

153d56e

rnettleton suggested changes Mar 9, 2018

View reviewed changes

adoroszlai assigned benyoka Mar 9, 2018

adoroszlai added the blueprint label Mar 9, 2018

AMBARI-23130 topology request mpack information normalized (benyoka)

80f0de6

Balazs Bence Sari added 2 commits March 13, 2018 15:46

Merge branch 'branch-feature-AMBARI-14714' into AMBARI-23130-branch-f…

512823e

…eature-AMBARI-14714

AMBARI-23130 fix broken unit test (benyoka)

f1302f7

rnettleton approved these changes Mar 14, 2018

View reviewed changes

jonathan-hurley reviewed Mar 14, 2018

View reviewed changes

jonathan-hurley approved these changes Mar 14, 2018

View reviewed changes

AMBARI-23130 fix import and review comments

d28589c

benyoka merged commit 0252c08 into apache:branch-feature-AMBARI-14714 Mar 19, 2018

AMBARI-23130 Persist mpack information instead of the full provision request #603

AMBARI-23130 Persist mpack information instead of the full provision request #603

Uh oh!

Conversation

benyoka commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benyoka Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnettleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Mar 9, 2018

Uh oh!

asfgit commented Mar 9, 2018

Uh oh!

asfgit commented Mar 10, 2018

Uh oh!

benyoka commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfgit commented Mar 13, 2018

Uh oh!

rnettleton commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

benyoka commented Mar 19, 2018

Uh oh!

asfgit commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benyoka commented Mar 9, 2018 •

edited

Loading

benyoka Mar 9, 2018 •

edited

Loading

benyoka commented Mar 13, 2018 •

edited

Loading