[AMBARI-22878] Update Service Group API to take list of mpack name associated with the service group #234

scottduan · 2018-01-30T21:01:33Z

What changes were proposed in this pull request?

When we post the REST api to create servicegroups, each sericegroup must include one and only one mpack name.

How was this patch tested?

(1) I have written an unit test to verify it
(2) I deployed a 3-node cluster and manually tested with REST API. This patch will break cluster deployment from webUI, so webUI needs another patch to support this change.

…ociated with the service group

scottduan · 2018-01-30T21:07:38Z

@jayush @mradha25

scottduan · 2018-01-30T21:09:32Z

@swapanshridhar

asfgit · 2018-01-30T22:38:04Z

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

adoroszlai · 2018-01-30T22:43:49Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java

+    if (!mpackNames.isEmpty()) {
+      sb.append(",mpackNames=");
+      mpackNames.stream().map(mpackName -> sb.append(mpackName + ","));
+      sb.deleteCharAt(sb.length()-1);


I think simply using built-in Set.toString() would be OK. If you prefer explicit formatting, instead of map(append) please use Collectors.joining, which is side-effect free, and also makes deleteCharAt unnecessary.

adoroszlai · 2018-01-30T22:44:59Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java

  public String toString() {
-    return String.format("clusterName=%s, serviceGroupName=%s", clusterName, serviceGroupName);
+    StringBuilder sb = new StringBuilder();
+    sb.append("clusterName="+clusterName+",serviceGroupName="+serviceGroupName);


Please avoid + inside append().

adoroszlai · 2018-01-30T22:49:20Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java

+   * @param mpackNames a list of associated mpack names
+   */
+  public void setMpackNames(Set<String> mpackNames) {
+    this.mpackNames.addAll(mpackNames);


The method name says set, but it adds to existing ones.

You are right. I copied and wrote,but forgot to update the name.

adoroszlai · 2018-01-30T23:16:13Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

    ServiceGroupRequest svcRequest = new ServiceGroupRequest(clusterName, serviceGroupName);
+    Set<Object> mpackNames = (Set<Object>) properties.get(SERVICE_GROUP_SERVICE_GROUP_MPACKNAME_PROPERTY_ID);
+    if (mpackNames != null) {
+      Set<String> mpackNamesSet = mpackNames.stream().flatMap(mpack -> ((Map<String, String>)mpack).values().stream()).collect(Collectors.toSet());


Can you minimize the number of unchecked casts by avoiding the intermediate Set<Object> type, and casting to Set<Map<String,String>> (or better yet, Collection<Map<String,String>>) in the first place?

adoroszlai · 2018-01-30T23:20:48Z

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

  private List<ServiceGroupDependencyEntity> dependencies;

+  @ElementCollection
+  @Column(name = "name")


Don't we need a corresponding change in the DDL scripts?
(ambari-server/src/main/resources/Ambari-DDL-*-CREATE.sql)

ERROR: relation "servicegroupentity_mpacknames" does not exist

adoroszlai · 2018-01-30T23:30:08Z

ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroup.java

+
+  void addServiceGroupMpackNames(Set<String> mpackNames);
+
+  void setServiceGroupMpackNames(Set<String> mpackNames);


Can you please omit ServiceGroup from the method names?

Seems other methods have ServiceGroup, anyway, I will remove it.

adoroszlai · 2018-01-30T23:30:50Z

ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroupImpl.java

+    if (!mpackNames.isEmpty()) {
+      sb.append(", mpackNames=");
+      mpackNames.stream().map(mpackName ->sb.append(mpackName + ","));
+      sb.deleteCharAt(sb.length()-1);


Same as above for similar logic in ServiceGroupRequest.toString().

adoroszlai · 2018-01-30T23:47:53Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

      serviceGroupNames.get(clusterName).add(serviceGroupName);

+      if (request.getMpackNames().size() != 1) {
+        String errmsg = "Invalid arguments, only one mpack is allowed in the service group " + serviceGroupName;


Please distinguish between 0 mpacks and 2+ mpacks. The error message "only one mpack is allowed" is a bit confusing when the request doesn't specify any mpacks for the service group.

I did not write the description of this bug clearly. At this time, temporarily we only support one mpack per servicegroup, but finally each servicegroup must have one or more mpacks when multiple mpacks support code is implemented. Again, I will add more clearer errmsg here.

if (request.getMpackNames().size() < 1)
Error should be that "Invalid arguments, atleast one mpack should be specified for a service group"
From backend, we should not restrict the number of mpacks associated to a service group.
@jayush , please correct me if wrong.

adoroszlai · 2018-01-30T23:54:26Z

...test/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProviderTest.java

+    System.out.println("++++++ Test invalid request successfully ++++++");
+  }
+
+  private static void runTestCreateServiceGroupsWithMpacks(String body) throws Exception{


If the same sequence is OK for both valid and invalid request, then what is being tested?

For invalid request, this is the negative test. As I mentioned above, actually at this time we only support 1 mpck per servicegroup. So in invalid test case, if we can detect it is invalid request, this test case succeeds. I will update the code to make it more clear.

mradha25 · 2018-01-31T20:13:30Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

  public static final String SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "cluster_name";
  public static final String SERVICE_GROUP_SERVICE_GROUP_ID_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "service_group_id";
  public static final String SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "service_group_name";
+  public static final String SERVICE_GROUP_SERVICE_GROUP_MPACKNAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "mpacks";


Can we simply call it SERVICE_GROUP_MPACKNAME_PROPERTY_ID?

We need to add this property to -

static {
// properties
https://github.com/apache/ambari/pull/234/files#diff-b9a4d90adcd0c31956baab7c90540e53R107

To the response of createResourcesAuthorized
https://github.com/apache/ambari/pull/234/files#diff-b9a4d90adcd0c31956baab7c90540e53L162

To the response of getResourcesAuthorized
https://github.com/apache/ambari/pull/234/files#diff-b9a4d90adcd0c31956baab7c90540e53R202

OK, I will change it to SERVICE_GROUP_MPACKNAME_PROPERTY_ID

mradha25 · 2018-01-31T20:31:20Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

      serviceGroupNames.get(clusterName).add(serviceGroupName);

+      if (request.getMpackNames().size() != 1) {
+        String errmsg = "Invalid arguments, only one mpack is allowed in the service group " + serviceGroupName;


if (request.getMpackNames().size() < 1)
Error should be that "Invalid arguments, atleast one mpack should be specified for a service group"
From backend, we should not restrict the number of mpacks associated to a service group.
@jayush , please correct me if wrong.

…ociated with the service group

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

asfgit · 2018-02-02T01:03:40Z

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

adoroszlai

Thanks for addressing previous comments.

adoroszlai · 2018-02-02T07:10:17Z

...test/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProviderTest.java

+    try {
+      runTestCreateServiceGroupsWithMpacks(body);
+    } catch (IllegalArgumentException e) {
+      System.out.println("++++++ Test invalid request successfully ++++++");


Thanks, the test is much clearer now for the reader. However, unit tests should fail in case of unexpected results, not print a message. Can you please:

extract the 2 test cases into separate methods

get rid of the try-catch in both cases

add @Test(expected = IllegalArgumentException.class) for the invalid request case

This will allow the test suite to catch any regression automatically, instead of relying on the developer to run it and look for text output. Thanks.

adoroszlai · 2018-02-02T07:53:26Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

      }
      serviceGroupNames.get(clusterName).add(serviceGroupName);

+      if (request.getMpackNames().size() != 1) {


Since this change introduces mpacks for service groups, and it also makes exactly one mpack a requirement, it breaks existing code that uses ServiceGroupResourceProvider to create mpacks. For one, it increases the number of failing/erroring unit tests:

[ERROR] Tests run: 5122, Failures: 39, Errors: 231, Skipped: 37 -> [ERROR] Tests run: 5123, Failures: 42, Errors: 296, Skipped: 37

Can you please address those? I think most could be fixed by adding some dummy mpack name in

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

Lines 39 to 43 in 00f29b9

public static void createServiceGroup(AmbariManagementController controller, String clusterName, String serviceGroupName)

throws AmbariException, AuthorizationException {

ServiceGroupRequest request = new ServiceGroupRequest(clusterName, serviceGroupName);

createServiceGroups(controller, Collections.singleton(request));

}

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

asfgit · 2018-02-05T08:39:18Z

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

scottduan · 2018-02-05T15:43:28Z

@swapanshridhar

adoroszlai · 2018-02-05T17:48:23Z

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

+  mpack_name VARCHAR(255) NOT NULL,
+  CONSTRAINT PK_servicegroup_mpackname PRIMARY KEY (service_group_id, service_group_cluster_id, mpack_name),
+  FOREIGN KEY (service_group_id) REFERENCES servicegroups(service_group_id),
+  FOREIGN KEY (service_group_cluster_id) REFERENCES servicegroups(service_group_cluster_id)


Constraints/foreign keys in this table are different than in the other DBMS schemas.

will update. Seems I forgot to update it.

adoroszlai · 2018-02-05T17:49:05Z

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

+  service_group_cluster_id BIGINT NOT NULL,
+  mpack_name VARCHAR(255) NOT NULL UNIQUE,
+  CONSTRAINT PK_servicegroup_mpacknames PRIMARY KEY (service_group_id, service_group_cluster_id, mpack_name),
+  CONSTRAINT UQ_servicegroup_mpacknames UNIQUE (service_group_id, service_group_cluster_id),


These unique constraints will require upgrade logic for dropping them when Ambari wants to allow multiple mpacks per service group.

I will remove mapck_name unique constraint. Finally we should allow many-many relationship between servicegroup and mpack.

adoroszlai · 2018-02-05T17:57:39Z

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

  FOREIGN KEY (history_id) REFERENCES alert_history(alert_id)
 );

+CREATE TABLE servicegroup_mpackname (


Table name is different in this schema (servicegroup_mpackname vs servicegroup_mpacknames).

Typo, will change it.

adoroszlai · 2018-02-05T17:58:36Z

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

+  CONSTRAINT PK_servicegroup_mpacknames PRIMARY KEY (service_group_id, service_group_cluster_id, mpack_name),
+  CONSTRAINT UQ_servicegroup_mpacknames UNIQUE (service_group_id, service_group_cluster_id),
+  CONSTRAINT FK_servicgroups FOREIGN KEY (service_group_id, service_group_cluster_id) REFERENCES servicegroups(id, cluster_id) ON DELETE CASCADE
+);DatabaseConsistencyCheckHelper


DatabaseConsistencyCheckHelper does not seem to belong here, and will cause syntax error.

I donot know where it comes from. I will delete it.

adoroszlai · 2018-02-05T18:07:09Z

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

+  @CollectionTable(name = "servicegroup_mpacknames",
+                   joinColumns = {@JoinColumn(name = "service_group_id", referencedColumnName = "id"),
+                                  @JoinColumn(name = "service_group_cluster_id", referencedColumnName = "cluster_id")})
+  @Column(name = "mpack_name", unique = true, nullable = false)


I don't think the mpack_name in itself should be unique. This causes unit test failures with the current constant "dummy" mpack name for tests. (eg. AmbariManagementControllerTest)

Actually mpack_name should not be unique. It should allow different service group to have the same mpack_name. I will remove this contraint.

jonathan-hurley

The SG -> Mpack association should be ManyToOne so that a given SG has a direct association to 1 (and only 1) Mpack at a given point. Something like this:

+  mpack_id BIGINT NOT NULL,
   CONSTRAINT PK_servicegroups PRIMARY KEY (id, cluster_id),
-  CONSTRAINT FK_servicegroups_cluster_id FOREIGN KEY (cluster_id) REFERENCES clusters (cluster_id));
+  CONSTRAINT FK_servicegroups_cluster_id FOREIGN KEY (cluster_id) REFERENCES clusters (cluster_id),
+  CONSTRAINT FK_servicegroups_mpack_id FOREIGN KEY (mpack_id) REFERENCES mpacks (id));

ncole

It seems like there's a design issue here. MPacks have to be 1:1 to SG. In addition, you have to associate MPacks by ID. You can't just say "SG for HDPCORE" - if you have more than one registered, which one do you pick? We have to design this relationship a bit more tightly.

ncole · 2018-02-05T19:46:20Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java

  @Override
  public String toString() {
-    return String.format("clusterName=%s, serviceGroupName=%s", clusterName, serviceGroupName);
+    StringBuilder sb = new StringBuilder();


We've been gradually moving to ToStringBuilder here instead

Yes, we should switch this to ToStringBuilder.

ncole · 2018-02-05T19:48:28Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

  public static final String SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "cluster_name";
  public static final String SERVICE_GROUP_SERVICE_GROUP_ID_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "service_group_id";
  public static final String SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "service_group_name";
+  public static final String SERVICE_GROUP_MPACKNAME_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "mpacks";


This is an awkward construct. Either do the string directly or use PropertyHelper.getPropertyId(...).

@ncole This is useful for when we document the API using Swagger annotations. The compiler does not allow using strings generated by getPropertyId() method call in annotation parameters. Using the string directly in multiple places is prone to typos. Hence this awkward construct was born.

Anyway, this question does not have much to do with this PR, as @scottduan just followed the existing usage pattern in the lines above.

I think mpack:sg is many to many relationship finally. At this time, we just have temporary restriction for 1:1, also I did not validate mpack information. This will be done in the following mpack work.

I don't think it should be M2M - it should be M2O. Since upgrade workflow is based off of SGs, it doesn't make sense to have the ability to mix them at this point. It creates more complexity that will not be used.

Also, this relationship shouldn't be based on names, but on specific IDs of the mpack. There shouldn't be ambiguity about which MPack a SG is based off of.

@adoroszlai - it's a constant, spelling should not be an issue. You can't use constants in Swagger? That would be extremely disappointing.

@jayush , how do you think? Based on my understand, SG can pick up service from different mpacks, so this should be M2M.
@jonathan-hurley , because we do not consider version. If we use mpack id, that means we also consider mpack version. As far as I know, now we only consider mpack name, not including version.

A SG should not be able to pickup services from different mpacks - it will break a lot of upgrade flow and assumptions. Additionally, it needs to be associated with a specific mpack in order to provide a way of querying what SG's are compatible with given mpack.

mpack provides with service dependence list. So when SG picks up service, theoratically it can choose proper service with the right version (it should be a range). There has a mpack design doc, https://docs.google.com/document/d/13y2T_uN8_nrMsNRuqXLnQAm64Edie0k8_WVNOFvKq0U/edit#heading=h.ykmhjj9wbfa8
We may need to schedule a meeting to clarify how to implement mpack feature in HDP3.

@ncole It's a limitation of annotation "element value" in Java, not specific to Swagger. The value must be a constant expression, not just any runtime constant.

ncole · 2018-02-05T19:49:45Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

      serviceGroupNames.get(clusterName).add(serviceGroupName);

+      if (request.getMpackNames().size() != 1) {
+        String errmsg = "Invalid arguments, " + request.getMpackNames().size() + " mpack(s) found in the service group " + serviceGroupName + ", only one mpack is allowed";


Use String.format() here.

It seems that concatenation has better performance than string.format().

For messages like this, we're more interested in readability in code than performance. We've been using String.format() in more and more places to be a bit more consistent with logging syntax (even if they are not identical).

Ok, I will update it.

Should I use StringBuilder instead? I saw your another comment mentioned it.

ncole · 2018-02-05T19:50:06Z

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

  @OneToMany(mappedBy="serviceGroupDependency")
  private List<ServiceGroupDependencyEntity> dependencies;

+  @ElementCollection()


Can you explain a little more there? Format what? Thanks.

I may have meant the next lines down - seems like a lot of whitespace.

I will remove all those whitespace, and put them on the same line.

…nto AMBARI-22878-branch-feature-AMBARI-14714

asfgit · 2018-02-09T05:40:00Z

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

…nto AMBARI-22878-branch-feature-AMBARI-14714

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

…nto AMBARI-22878-branch-feature-AMBARI-14714

scottduan · 2018-02-09T23:50:12Z

Just pushed a new patch. The manual test:

POST http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups
[
{
"ServiceGroupInfo": {
"service_group_name": "CORE2",
"stack_id": "HDP-1.2.3"
}
},
{
"ServiceGroupInfo": {
"service_group_name": "EDW-MKTG",
"stack_id": "EWD-1.1.1"
}
}
]

Response:
{
"resources" : [
{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups/CORE2",
"ServiceGroupInfo" : {
"cluster_id" : 102,
"cluster_name" : "c1",
"service_group_id" : 54,
"service_group_name" : "CORE2",
"stack_id" : "HDP-1.2.3"
}
},
{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups/EDW-MKTG",
"ServiceGroupInfo" : {
"cluster_id" : 102,
"cluster_name" : "c1",
"service_group_id" : 53,
"service_group_name" : "EDW-MKTG",
"stack_id" : "EWD-1.1.1"
}
}
]
}

For GET http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups

{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups",
"items" : [
{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups/EDW-MKTG",
"ServiceGroupInfo" : {
"cluster_name" : "c1",
"service_group_name" : "EDW-MKTG",
"stack_id" : "EWD-1.1.1"
}
},
{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups/CORE2",
"ServiceGroupInfo" : {
"cluster_name" : "c1",
"service_group_name" : "CORE2",
"stack_id" : "HDP-1.2.3"
}
},
{
"href" : "http://c6801.ambari.apache.org:8080/api/v1/clusters/c1/servicegroups/core",
"ServiceGroupInfo" : {
"cluster_name" : "c1",
"service_group_name" : "core",
"stack_id" : "HDP-3.0.0.0"
}
}
]
}

scottduan · 2018-02-09T23:51:35Z

The relation between servicegroup and mpack(stack-id) is mang-to-one, each servicegroup can have only one mpack. The same mpack can be used in multiple servicegroups

asfgit · 2018-02-10T02:51:11Z

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

…nto AMBARI-22878-branch-feature-AMBARI-14714

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

asfgit · 2018-02-16T07:06:20Z

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

jonathan-hurley · 2018-02-16T15:15:25Z

...src/main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java

+        stackVersion = "HDP-2.0.6";
+        request.setStackId(stackVersion);
+        LOG.info("***** Add fake stack id in order to create cluster from web UI, this code will be removed after web UI adds default stack id");
+      }


Why is this fake code needed? If the web client is sending down any information about mpacks, then you should be able to use that, right? If no, then just get the first mpack registered for now.

Don't hardcode this. Take the value from cluster/version to make it work for single mpack install for now.

jonathan-hurley · 2018-02-16T15:16:42Z

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


+  @Column(name = "stack_id", nullable = false, insertable = true, updatable = true)
+  private Long stackId;
+


I think we need to make this an entity relationship instead since it's most likely that we'll need the entity data when dealing with service groups.

this stackId is the foreign key of stack table. We can get stack entity from this key.

That is true, but this means we'd need an extra step, like this:

@Inject StackDAO stackDAO; stackDAO.findById(ServiceGroupEntity.getStackId())

vs just
ServiceGroupEntity.getStackEntity();

I guess I'm OK leaving it, but why not change it now before consumers begin using it?

jonathan-hurley · 2018-02-16T15:18:06Z

ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroup.java


+  String getStackId();
+
+  void setStackId(String stackId);


Add documentation here. "StackID" can mean anything - it should be explicitly stated that a Stack ID represents a specific mpack version: MPackName-version

So, for example:
Foo-1.0.0.0-b25
Bar-3.1.0.4-b2

In ambari code, StackId is widely used as the combination of stackName-stackVersion, there has a class StackId is for this, so I do not think it is caused confusion.

Why not use StackId then instead of String?

In Ambari < 3.0, the Stack ID was typically a 2-digit umbrella, like "HDP 2.6". It did not represent unique versions, like HDP 2.6.0.0-1234. We're changing this now so that Stack ID refers to the unique, fully-qualified version. I guess that's why it's a bit confusing to me.

jonathan-hurley

I think the term StackId is getting overloaded and is becoming quite confusing.

A stackId is a foreign key reference to a specific entry in the stack table. Each entry in the stack table also has a foreign key to a unique mpack. We need to make this a non-NULL key as well.

Whatever we call the combination of stack name and stack version, it shouldn't be stackId, since that indicates something else.

jonathan-hurley · 2018-02-16T15:19:11Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java

+  private String stackId; // Associated stack version info

-  public ServiceGroupRequest(String clusterName, String serviceGroupName) {
+  public ServiceGroupRequest(String clusterName, String serviceGroupName, String stackId) {


I think using the term "stackId" is a bit confusing here - it should be differentiated from the "stackId" foreign key value. In this case, it's actually the Mpack Name and specific MPack version (with build number), right?

scottduan · 2018-02-16T20:29:52Z

@jonathan-hurley @jayush I need to clarify what information should be saved in servicegroups? If api only passes in version which is stackName+stackVersion, backend can get stackId by querfying stack table with stackname and stackversion, so servicegroup table can save stack_id(real stack id), the mpack info can also get from quarified stack entity, so I do not think it is necessary to save mpack info in servicegroup. If that is ok, I will keep my current change in servicegroups table schema. The only change is to use "version" to replace "stackId" to avoid confustion. Also this is for 3.0+, not for 2.7-.

jonathan-hurley · 2018-02-16T20:49:03Z

At the end of the day, the service group must have a direct, foreign key relationship to an entry in the stack table. The stack table, in turn, has a direct foreign key relationship to a single mpack. So, one can do this:

ServiceGroupEntity.getStackEntity().getMpackEntity()

As long as this is satisfied, then we're good. We don't want to do extra queries in the DAO by name/version since this causes a hit agains the DB instead of object cache.

As for the Ambari 3.0/2.7 ... yes, I know that this work is for 3.0, however we still need to operate on older style stacks after upgrade. The term stackId was being overloaded (in some cases it meant version, in some cases an ID, etc) ... I just wanted it to be a bit clearer, which is either documentation or some name changes.

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

scottduan · 2018-02-18T01:23:33Z

The updated patch includes:

Remove StackId, instead use version to represent "stackName-stackVersion" to avoid any confusion with stack_id;
Follows Jonathan's suggestion, a new API in servicegroup is implemented: StackEntity getStackEntity(), so that we can get mapck info from stackEntity and from serviceGroup.

jonathan-hurley · 2018-02-20T13:34:02Z

ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroupImpl.java

    this.serviceGroupName = serviceGroupName;
+    // version is actually stackName-stackVersion
+    int idx = version.indexOf('-');
+    StackId stackId = new StackId(version.substring(0, idx), version.substring(idx+1));


I think there's still a little bit of disconnect - I think it would be clearer to separate out the mpack name and the mpack version into separate fields. That way, we don't need to parse dashes and things like that.

Also, you could just use the ctor of StackId that does the parsing for you

jonathan-hurley · 2018-02-20T13:35:24Z

ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java


-  public ServiceGroupRequest(String clusterName, String serviceGroupName) {
+  public ServiceGroupRequest(String clusterName, String serviceGroupName, String version) {
    this.clusterName = clusterName;


I think there's still a misunderstanding here - I thought it would be cleaner to separate this out into 2 fields so that it's clear to the user what is expected: mpack name and then mpack version. You can combine this and call it a stack ID internally, that's fine - but we should take 2 different properties.

Same goes for API requests where we're currently given name-version as a string. This should be broken out into 2 fields.

I am ok for this change.

@jayush What do you think?

Lets keep it as "version" for now for legacy purposes. When we upgrade to ambari 3.0 assuming the existing cluster is on HDP-2.6 stack, calling this as mpack name and mpack version would be misleading.

We should handle this in a separate JIRA if we really want to change this convention for all APIs.

But "version" in legacy terms meant only the numeric value. If we don't want to break it out into 2 references, I think we'd rather use stackId, right?

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

scottduan · 2018-02-22T17:56:47Z

@jonathan-hurley @adoroszlai @ncole I have updated the patch and could you please review it again? Thanks.

jayush · 2018-02-22T18:56:35Z

ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java

  @Override
-  public ServiceGroup addServiceGroup(String serviceGroupName) throws AmbariException {
+  public ServiceGroup addServiceGroup(String serviceGroupName, String version) throws AmbariException {
    if (serviceGroups.containsKey(serviceGroupName)) {


addServiceGroup(serviceGroupName, stackId) already has the same check. Remove duplicate code.

I will remove the check in addServiceGroup(String serviceGroupName, String version)

jayush · 2018-02-22T18:57:20Z

ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java

   * @throws AmbariException
   */
-  ServiceGroup addServiceGroup(String serviceGroupName) throws AmbariException;
+  ServiceGroup addServiceGroup(String serviceGroupName, String stackId) throws AmbariException;


Rename "String stackId" to "String version" in the interface.

The change is already there.

ok, changed it.

jayush · 2018-02-22T18:58:51Z

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


    Set<ServiceGroupRequest> serviceGroupRequests = serviceGroups.stream()
-      .map(serviceGroupName -> new ServiceGroupRequest(clusterName, serviceGroupName))
+      .map(serviceGroupName -> new ServiceGroupRequest(clusterName, serviceGroupName, Iterables.getFirst(topology.getBlueprint().getStackIds(), null).getStackId()))


@adoroszlai do we have a follow up JIRA for this?

jonathan-hurley

OK - this commit is far along enough where it can go in since there are Jiras which are blocked by it. However, I think we need to address the version vs stackId discussion.

My original request was to make the one value broken out into 2 - mpack name and version. Instead, we made it just "version" which I think is more confusing than StackId. If we don't want to break it out into 2 properties, can we just move it back to stackID to be consistent?

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

asfgit · 2018-02-22T23:39:32Z

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

asfgit · 2018-02-23T00:23:26Z

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

AMBARI-22878: Update Service Group API to take list of mpack name ass…

dcb871b

…ociated with the service group

adoroszlai suggested changes Jan 31, 2018

View reviewed changes

adoroszlai requested review from jayush and mradha25 January 31, 2018 00:09

adoroszlai assigned scottduan Jan 31, 2018

mradha25 suggested changes Jan 31, 2018

View reviewed changes

scottduan added 2 commits February 1, 2018 15:22

AMBARI-22878: Update Service Group API to take list of mpack name ass…

e97eb29

…ociated with the service group

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

da41ca0

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

adoroszlai suggested changes Feb 2, 2018

View reviewed changes

scottduan added 2 commits February 4, 2018 22:21

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

158cd85

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

decbf54

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

jayush requested a review from swapanshridhar February 5, 2018 15:43

adoroszlai suggested changes Feb 5, 2018

View reviewed changes

jonathan-hurley self-requested a review February 5, 2018 19:48

jonathan-hurley suggested changes Feb 5, 2018

View reviewed changes

ncole suggested changes Feb 5, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

68c1f04

…nto AMBARI-22878-branch-feature-AMBARI-14714

scottduan added 3 commits February 9, 2018 15:35

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

a2340a6

…nto AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

04345f8

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

90f354f

…nto AMBARI-22878-branch-feature-AMBARI-14714

ishanbha force-pushed the branch-feature-AMBARI-14714 branch from 6436488 to 260876c Compare February 13, 2018 00:03

scottduan and others added 2 commits February 15, 2018 22:04

Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…

a331735

…nto AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

3de0e93

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

jonathan-hurley reviewed Feb 16, 2018

View reviewed changes

jonathan-hurley suggested changes Feb 16, 2018

View reviewed changes

Scott Duan added 2 commits February 17, 2018 16:36

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

f74af11

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

afc6a3b

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

jonathan-hurley reviewed Feb 20, 2018

View reviewed changes

Scott Duan added 2 commits February 22, 2018 09:54

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

fb3e005

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

7bcd6fe

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

jayush reviewed Feb 22, 2018

View reviewed changes

jonathan-hurley approved these changes Feb 22, 2018

View reviewed changes

Scott Duan added 2 commits February 22, 2018 11:16

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

cfe5295

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…

d370602

…thub.com/scottduan/ambari into AMBARI-22878-branch-feature-AMBARI-14714

jayush approved these changes Feb 22, 2018

View reviewed changes

jayush merged commit d370602 into apache:branch-feature-AMBARI-14714 Feb 22, 2018

adoroszlai mentioned this pull request Feb 23, 2018

[AMBARI-22878] Fix unit tests broken by reference to non-existent stack #449

Merged

This was referenced Feb 26, 2018

AMBARI-22878. Fix unit tests #473

Merged

AMBARI-22945. Enhance host components API to support multiple host component instances. #467

Closed

scottduan deleted the AMBARI-22878-branch-feature-AMBARI-14714 branch March 15, 2018 05:14


		void addServiceGroupMpackNames(Set<String> mpackNames);

		void setServiceGroupMpackNames(Set<String> mpackNames);

	public static void createServiceGroup(AmbariManagementController controller, String clusterName, String serviceGroupName)
	throws AmbariException, AuthorizationException {
	ServiceGroupRequest request = new ServiceGroupRequest(clusterName, serviceGroupName);
	createServiceGroups(controller, Collections.singleton(request));
	}


		@Column(name = "stack_id", nullable = false, insertable = true, updatable = true)
		private Long stackId;

[AMBARI-22878] Update Service Group API to take list of mpack name associated with the service group #234

[AMBARI-22878] Update Service Group API to take list of mpack name associated with the service group #234

Uh oh!

Conversation

scottduan commented Jan 30, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

scottduan commented Jan 30, 2018

Uh oh!

scottduan commented Jan 30, 2018

Uh oh!

asfgit commented Jan 30, 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!

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!

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!

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!

asfgit commented Feb 2, 2018

Uh oh!

adoroszlai left a comment

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!

asfgit commented Feb 5, 2018

Uh oh!

scottduan commented Feb 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jonathan-hurley left a comment •

edited

Loading

jonathan-hurley Feb 6, 2018 •

edited

Loading