Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jayush merged 17 commits into from
Feb 22, 2018
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
dcb871b
AMBARI-22878: Update Service Group API to take list of mpack name ass…
scottduan Jan 30, 2018
e97eb29
AMBARI-22878: Update Service Group API to take list of mpack name ass…
scottduan Jan 30, 2018
da41ca0
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
scottduan Feb 1, 2018
158cd85
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
scottduan Feb 1, 2018
decbf54
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
scottduan Feb 5, 2018
68c1f04
Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…
scottduan Feb 9, 2018
a2340a6
Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…
scottduan Feb 9, 2018
04345f8
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
scottduan Feb 9, 2018
90f354f
Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…
scottduan Feb 9, 2018
a331735
Merge remote-tracking branch 'upstream/branch-feature-AMBARI-14714' i…
scottduan Feb 9, 2018
3de0e93
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 16, 2018
f74af11
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 16, 2018
afc6a3b
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 18, 2018
fb3e005
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 18, 2018
7bcd6fe
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 22, 2018
cfe5295
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 22, 2018
d370602
Merge branch 'AMBARI-22878-branch-feature-AMBARI-14714' of https://gi…
Feb 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,20 @@
*/
package org.apache.ambari.server.controller;

import java.util.HashSet;
import java.util.Objects;
import java.util.Set;

public class ServiceGroupRequest {

private String clusterName; // REF
private String serviceGroupName; // GET/CREATE/UPDATE/DELETE
private String stackId; // Associated stack version info

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

@jonathan-hurley jonathan-hurley Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by this. Why does a service group have a stack association? An mpack has the knowledge of which stack it belongs to, so the service group should have an mpack instance association.

@jayush jayush Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathan-hurley
Our DB schemas are all coupled to stack-id. The only association we have between stack and mpack is in the stacks table. We will keep the association to stack-id to keep things consistent as well as to support creating service groups for old stacks

@jonathan-hurley jonathan-hurley Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But multiple mpacks could have the same stack ID, right? How does this allow us to determine which mpack a service group is associated with?

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jayush jayush Feb 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just call the string as "version" like we do current for cluster.

POST /api/v1/clusters
{
"Clusters" : {
"cluster_name": "cl1",
"version" : "HDP-2.5"
}
}

@scottduan scottduan Feb 16, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use version instead of stackId on rest api layer, and internally use stackId instance to represent stackName-stackVersion.
@jonathan-hurley you are right, the only association between stack and mpack is in stack table, so if servicegroup has stackName+stackVersion, it can identify stack entity and then can get Mapck entity.(name, version etc)

this.clusterName = clusterName;

@jonathan-hurley jonathan-hurley Feb 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@scottduan scottduan Feb 21, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok for this change.

@scottduan scottduan Feb 21, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayush What do you think?

@jayush jayush Feb 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jonathan-hurley jonathan-hurley Feb 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

this.serviceGroupName = serviceGroupName;
this.stackId = stackId;
}

/**
Expand Down Expand Up @@ -57,9 +61,25 @@ public void setServiceGroupName(String serviceGroupName) {
this.serviceGroupName = serviceGroupName;
}

/**
* @return the stackId
*/
public String getStackId() {
return stackId;
}

/**
* @param stackId the stackId to set
*/
public void setStackId(String stackId) {
this.stackId = stackId;
}

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

@ncole ncole Feb 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been gradually moving to ToStringBuilder here instead

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should switch this to ToStringBuilder.

sb.append("clusterName=").append(clusterName).append(", serviceGroupName=").append(serviceGroupName).append(", stackId=").append(stackId);
return sb.toString();
}

@Override
Expand All @@ -73,12 +93,11 @@ public boolean equals(Object obj) {

ServiceGroupRequest other = (ServiceGroupRequest) obj;

return Objects.equals(clusterName, other.clusterName) &&
Objects.equals(serviceGroupName, other.serviceGroupName);
return Objects.equals(clusterName, other.clusterName) && Objects.equals(serviceGroupName, other.serviceGroupName) && Objects.equals(stackId, other.stackId);
}

@Override
public int hashCode() {
return Objects.hash(clusterName, serviceGroupName);
return Objects.hash(clusterName, serviceGroupName, stackId);
}
}
27 changes: 26 additions & 1 deletion ambari-server/src/main/java/org/apache/ambari/server/controller/ServiceGroupResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@

package org.apache.ambari.server.controller;

import java.util.HashSet;
import java.util.Set;

import io.swagger.annotations.ApiModelProperty;

public class ServiceGroupResponse {
Expand All @@ -26,12 +29,15 @@ public class ServiceGroupResponse {
private Long serviceGroupId;
private String clusterName;
private String serviceGroupName;
private String stackId;

public ServiceGroupResponse(Long clusterId, String clusterName, Long serviceGroupId, String serviceGroupName) {
public ServiceGroupResponse(Long clusterId, String clusterName, Long serviceGroupId, String serviceGroupName, String stackId) {
this.clusterId = clusterId;
this.serviceGroupId = serviceGroupId;
this.clusterName = clusterName;
this.serviceGroupName = serviceGroupName;
this.stackId = stackId;

}

/**
Expand Down Expand Up @@ -90,6 +96,20 @@ public void setServiceGroupName(String serviceGroupName) {
this.serviceGroupName = serviceGroupName;
}

/**
* @return the stack ID (stack name - stack Id)
*/
public String getStackId() {
return stackId;
}

/**
* @param stackId the stack Id
*/
public void setStackId(String stackId) {
this.stackId = stackId;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand All @@ -110,6 +130,11 @@ public boolean equals(Object o) {
return false;
}

if (stackId != null ?
!stackId.equals(that.stackId) : that.stackId != null) {
return false;
}

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion ...java/org/apache/ambari/server/controller/internal/AbstractAuthorizedResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public void setRequiredDeleteAuthorizations(Set<RoleAuthorization> requiredDelet
*/
@Override
public RequestStatus createResources(Request request)
throws SystemException, UnsupportedPropertyException, ResourceAlreadyExistsException, NoSuchParentResourceException {
throws SystemException, UnsupportedPropertyException, ResourceAlreadyExistsException, NoSuchParentResourceException, IllegalArgumentException {
Authentication authentication = AuthorizationHelper.getAuthentication();

if (authentication == null || !authentication.isAuthenticated()) {
Expand Down
33 changes: 26 additions & 7 deletions .../main/java/org/apache/ambari/server/controller/internal/ServiceGroupResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
package org.apache.ambari.server.controller.internal;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import org.apache.ambari.server.AmbariException;
import org.apache.ambari.server.ClusterNotFoundException;
Expand Down Expand Up @@ -78,6 +80,7 @@ public class ServiceGroupResourceProvider extends AbstractControllerResourceProv
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_STACK_ID_PROPERTY_ID = RESPONSE_KEY + PropertyHelper.EXTERNAL_PATH_SEP + "stack_id";


private static Set<String> pkPropertyIds =
Expand All @@ -103,10 +106,12 @@ public class ServiceGroupResourceProvider extends AbstractControllerResourceProv
PROPERTY_IDS.add(SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID);
PROPERTY_IDS.add(SERVICE_GROUP_SERVICE_GROUP_ID_PROPERTY_ID);
PROPERTY_IDS.add(SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID);
PROPERTY_IDS.add(SERVICE_GROUP_STACK_ID_PROPERTY_ID);

// keys
KEY_PROPERTY_IDS.put(Resource.Type.Cluster, SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID);
KEY_PROPERTY_IDS.put(Resource.Type.ServiceGroup, SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID);
KEY_PROPERTY_IDS.put(Resource.Type.Stack, SERVICE_GROUP_STACK_ID_PROPERTY_ID);
}

private Clusters clusters;
Expand Down Expand Up @@ -136,7 +141,7 @@ protected RequestStatus createResourcesAuthorized(Request request)
throws SystemException,
UnsupportedPropertyException,
ResourceAlreadyExistsException,
NoSuchParentResourceException {
NoSuchParentResourceException, IllegalArgumentException{

final Set<ServiceGroupRequest> requests = new HashSet<>();
for (Map<String, Object> propertyMap : request.getProperties()) {
Expand All @@ -145,7 +150,7 @@ protected RequestStatus createResourcesAuthorized(Request request)
Set<ServiceGroupResponse> createServiceGroups = null;
createServiceGroups = createResources(new Command<Set<ServiceGroupResponse>>() {
@Override
public Set<ServiceGroupResponse> invoke() throws AmbariException, AuthorizationException {
public Set<ServiceGroupResponse> invoke() throws AmbariException, AuthorizationException, IllegalArgumentException {
return createServiceGroups(requests);
}
});
Expand All @@ -160,6 +165,7 @@ public Set<ServiceGroupResponse> invoke() throws AmbariException, AuthorizationE
resource.setProperty(SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID, response.getClusterName());
resource.setProperty(SERVICE_GROUP_SERVICE_GROUP_ID_PROPERTY_ID, response.getServiceGroupId());
resource.setProperty(SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID, response.getServiceGroupName());
resource.setProperty(SERVICE_GROUP_STACK_ID_PROPERTY_ID, response.getStackId());

associatedResources.add(resource);
}
Expand Down Expand Up @@ -199,6 +205,8 @@ public Set<ServiceGroupResponse> invoke() throws AmbariException {
response.getServiceGroupId(), requestedIds);
setResourceProperty(resource, SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID,
response.getServiceGroupName(), requestedIds);
setResourceProperty(resource, SERVICE_GROUP_STACK_ID_PROPERTY_ID,
response.getStackId(), requestedIds);
resources.add(resource);
}
return resources;
Expand Down Expand Up @@ -268,13 +276,14 @@ protected Set<String> getPKPropertyIds() {
private ServiceGroupRequest getRequest(Map<String, Object> properties) {
String clusterName = (String) properties.get(SERVICE_GROUP_CLUSTER_NAME_PROPERTY_ID);
String serviceGroupName = (String) properties.get(SERVICE_GROUP_SERVICE_GROUP_NAME_PROPERTY_ID);
ServiceGroupRequest svcRequest = new ServiceGroupRequest(clusterName, serviceGroupName);
String stackId = (String) properties.get(SERVICE_GROUP_STACK_ID_PROPERTY_ID);
ServiceGroupRequest svcRequest = new ServiceGroupRequest(clusterName, serviceGroupName, stackId);
return svcRequest;
}

// Create services from the given request.
public synchronized Set<ServiceGroupResponse> createServiceGroups(Set<ServiceGroupRequest> requests)
throws AmbariException, AuthorizationException {
throws AmbariException, AuthorizationException, IllegalArgumentException {

if (requests.isEmpty()) {
LOG.warn("Received an empty requests set");
Expand All @@ -291,7 +300,7 @@ public synchronized Set<ServiceGroupResponse> createServiceGroups(Set<ServiceGro
Cluster cluster = clusters.getCluster(request.getClusterName());

// Already checked that service group does not exist
ServiceGroup sg = cluster.addServiceGroup(request.getServiceGroupName());
ServiceGroup sg = cluster.addServiceGroup(request.getServiceGroupName(), request.getStackId());
createdSvcGrps.add(sg.convertToResponse());
}
return createdSvcGrps;
Expand Down Expand Up @@ -389,21 +398,29 @@ ResourceType.CLUSTER, getClusterResourceId(serviceGroupRequest.getClusterName())


private void validateCreateRequests(Set<ServiceGroupRequest> requests, Clusters clusters)
throws AuthorizationException, AmbariException {
throws AuthorizationException, AmbariException, IllegalArgumentException {

AmbariMetaInfo ambariMetaInfo = getManagementController().getAmbariMetaInfo();
Map<String, Set<String>> serviceGroupNames = new HashMap<>();
Set<String> duplicates = new HashSet<>();
for (ServiceGroupRequest request : requests) {
final String clusterName = request.getClusterName();
final String serviceGroupName = request.getServiceGroupName();
String stackVersion = request.getStackId();
//TODO: ******* For test ********* this piece of code should be removed after web UI adds default stack id
if (StringUtils.isBlank(stackVersion)) {
stackVersion = "HDP-3.0.0.0";
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");
}

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jayush jayush Feb 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Validate.notNull(clusterName, "Cluster name should be provided when creating a service group");
Validate.notEmpty(serviceGroupName, "Service group name should be provided when creating a service group");
Validate.notEmpty(stackVersion, "Stack version should be provided when creating a service group");

if (LOG.isDebugEnabled()) {
LOG.debug("Received a createServiceGroup request" +
", clusterName=" + clusterName + ", serviceGroupName=" + serviceGroupName + ", request=" + request);
", clusterName=" + clusterName + ", serviceGroupName=" + serviceGroupName + ", stackVersion=" + stackVersion + ", request=" + request);
}

if (!AuthorizationHelper.isAuthorized(ResourceType.CLUSTER,
Expand All @@ -422,6 +439,8 @@ private void validateCreateRequests(Set<ServiceGroupRequest> requests, Clusters
}
serviceGroupNames.get(clusterName).add(serviceGroupName);

// TODO: Validate stack version

Cluster cluster;
try {
cluster = clusters.getCluster(clusterName);
Expand Down
19 changes: 18 additions & 1 deletion ambari-server/src/main/java/org/apache/ambari/server/orm/entities/ServiceGroupEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@

package org.apache.ambari.server.orm.entities;

import java.util.HashSet;
import java.util.List;
import java.util.Set;

import javax.persistence.CollectionTable;
import javax.persistence.Column;
import javax.persistence.ElementCollection;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
Expand Down Expand Up @@ -70,6 +74,9 @@ public class ServiceGroupEntity {
@Column(name = "service_group_name", nullable = false, insertable = true, updatable = true)
private String serviceGroupName;

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

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@scottduan scottduan Feb 16, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ManyToOne
@JoinColumn(name = "cluster_id", referencedColumnName = "cluster_id", nullable = false)
private ClusterEntity clusterEntity;
Expand All @@ -96,7 +103,6 @@ public void setServiceGroupId(Long serviceGroupId) {
this.serviceGroupId = serviceGroupId;
}


public String getServiceGroupName() {
return serviceGroupName;
}
Expand All @@ -105,6 +111,14 @@ public void setServiceGroupName(String serviceGroupName) {
this.serviceGroupName = serviceGroupName;
}

public String getStackId() {
return stackId;
}

public void setStackId(String stackId) {
this.stackId = stackId;
}

public List<ServiceGroupDependencyEntity> getDependencies() {
return dependencies;
}
Expand All @@ -131,6 +145,8 @@ public boolean equals(Object o) {
if (clusterId != null ? !clusterId.equals(that.clusterId) : that.clusterId != null) return false;
if (serviceGroupName != null ? !serviceGroupName.equals(that.serviceGroupName) : that.serviceGroupName != null)
return false;
if (stackId != null ? !stackId.equals(that.stackId) : that.stackId != null)
return false;

@jonathan-hurley jonathan-hurley Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert to Objects.equals() or EqualsBuilder()


return true;
}
Expand All @@ -139,6 +155,7 @@ public boolean equals(Object o) {
public int hashCode() {
int result = clusterId != null ? clusterId.intValue() : 0;
result = 31 * result + (serviceGroupName != null ? serviceGroupName.hashCode() : 0);
result = 31 * result + (stackId != null ? stackId.hashCode() : 0);
return result;
}

Expand Down
2 changes: 1 addition & 1 deletion ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ Service addDependencyToService(String serviceGroupName, String serviceName,
* @return
* @throws AmbariException
*/
ServiceGroup addServiceGroup(String serviceGroupName) throws AmbariException;
ServiceGroup addServiceGroup(String serviceGroupName, String stackId) throws AmbariException;

@jayush jayush Feb 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scottduan scottduan Feb 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is already there.

@scottduan scottduan Feb 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed it.


/**
* Add service group dependency to the service group
Expand Down
4 changes: 4 additions & 0 deletions ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ public interface ServiceGroup {

Set<ServiceGroupDependencyResponse> getServiceGroupDependencyResponses();

String getStackId();

void setStackId(String stackId);

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scottduan scottduan Feb 16, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@adoroszlai adoroszlai Feb 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use StackId then instead of String?

@jonathan-hurley jonathan-hurley Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void debugDump(StringBuilder sb);

void refresh();
Expand Down
1 change: 1 addition & 0 deletions ambari-server/src/main/java/org/apache/ambari/server/state/ServiceGroupFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public interface ServiceGroupFactory {

ServiceGroup createNew(Cluster cluster,
@Assisted("serviceGroupName") String serviceGroupName,
@Assisted("stackId") String stackId,

@ncole ncole Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stackId should be the object StackId in this method

@adoroszlai adoroszlai Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for StackId

@scottduan scottduan Feb 16, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change to stackId instance

@Assisted("serviceGroupDependencies") Set<ServiceGroupKey> serviceGroupDependencies);

ServiceGroup createExisting(Cluster cluster, ServiceGroupEntity serviceGroupEntity);
Expand Down
Loading