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.
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
AMBARI-22945. Enhance host components API to support multiple host component instances. #477
Conversation
Refer to this link for build results (access rights to CI server needed): |
@Produces("text/plain") | ||
public Response createHostComponent(String body, @Context HttpHeaders headers, @Context UriInfo ui, | ||
@PathParam("hostComponentName") String hostComponentName) { | ||
@PathParam("hostComponentId") String hostComponentId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
For POST call we will not have hostComponentId in the PathParam since it has not been created yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this POST call.
@@ -677,6 +684,8 @@ protected RequestStageContainer updateHostComponents(RequestStageContainer stage | |||
+ ", clusterId=" + cluster.getClusterId() | |||
+ ", serviceName=" + sch.getServiceName() | |||
+ ", componentName=" + sch.getServiceComponentName() | |||
+ ", componentType=" + sch.getServiceComponentType() | |||
+ ", componentType=" + sch.getServiceComponentType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
ServiceComponentHostRequest serviceComponentHostRequest = null; | ||
if (properties.get(HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID) != null) { | ||
Long hostComponentId = properties.get(HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID) instanceof String ? | ||
Long.parseLong((String) properties.get(HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID)) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are checking if its an instance of String, and then casting it to String and then using parseLong. If we have already checked that its an instance of String then we should be able to simply do
Long.parseLong(properties.get(HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID)).
Might be simpler to just use :
Long hostComponentId = Long.valueOf((String) properties.get(HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID)
parseLong returns a primitive type so you can choose to use valueOf to get an object of Long type.
query.setParameter("clusterId", clusterId); | ||
query.setParameter("serviceGroupId", serviceGroupId); | ||
query.setParameter("serviceId", serviceId); | ||
query.setParameter("id", componentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need clusterId, serviceGroupId, serviceId in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it.
"AND scds.componentName = :componentName " + | ||
"AND scds.componentType = :componentType" ), | ||
@NamedQuery( | ||
name = "ServiceComponentDesiredStateEntity.findById", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id will already be unique. Do we need the clusterId, serviceGroupId and serviceId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it.
public String getComponentName(Long componentId) throws AmbariException { | ||
for (Service service : services.values()) { | ||
for (ServiceComponent component : service.getServiceComponents().values()) { | ||
if (component.getId() == componentId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use .equals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
public String getComponentType(Long componentId) throws AmbariException { | ||
for (Service service : services.values()) { | ||
for (ServiceComponent component : service.getServiceComponents().values()) { | ||
if (component.getId() == componentId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use .equals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
+ ", serviceType=" + serviceType | ||
+ ", serviceComponentId=" + serviceComponentId); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is serviceComponentId stored in the DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should come from servicecomponentdesiredstate table.
d6fc1cd
to
df4aa4b
Compare
Adds/changes the following:
Manual testing done as follows:
|
df4aa4b
to
93d949c
Compare
|
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@avijayanhwx Can you review this change for Metrics related bare minimum changes that I have done. // TODO : Multi_Metrics_Changes. where I have identified further code changes are required to uniquely identify one components from another. |
93d949c
to
2d7d560
Compare
Refer to this link for build results (access rights to CI server needed): |
2d7d560
to
34080a3
Compare
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics changes are OK for a preliminary patch. After implementing support for multi instance metrics in AMS, we can use a follow up JIRA to clean up metrics related integration points in Ambari Server.
try { | ||
componentId = managementController.getClusters().getCluster(clusterName).getComponentId(componentName); | ||
} catch (AmbariException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we log the exception instead of printStackTrace ?
@@ -70,7 +70,7 @@ public RootClusterSettingService() { | |||
response = ReadOnlyConfigurationResponse.ReadOnlyConfigurationResponseSwagger.class, responseContainer = RESPONSE_CONTAINER_LIST) | |||
@ApiImplicitParams({ | |||
@ApiImplicitParam(name = QUERY_FIELDS, value = QUERY_FILTER_DESCRIPTION, dataType = DATA_TYPE_STRING, | |||
paramType = PARAM_TYPE_QUERY, defaultValue = MpackResourceProvider.MPACK_RESOURCE_ID), | |||
paramType = PARAM_TYPE_QUERY, defaultValue = MpackResourceProvider.MPACK_ID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this line completely.
…mponent instances.
34080a3
to
2c844e8
Compare
(String) properties.get(HOST_COMPONENT_HOST_NAME_PROPERTY_ID), | ||
(String) properties.get(HOST_COMPONENT_DESIRED_STATE_PROPERTY_ID)); | ||
} | ||
|
||
serviceComponentHostRequest.setState((String) properties.get(HOST_COMPONENT_STATE_PROPERTY_ID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in NPE if HOST_COMPONENT_HOST_COMPONENT_ID_PROPERTY_ID
is not present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #566 and AMBARI-23158
Refer to this link for build results (access rights to CI server needed): |
What changes were proposed in this pull request?
BACKGROUND:
Given that in Ambari, we refer components of a services in 2 ways:
1. One from the Service component APIs :
http://:8080/api/v1/clusters//services//components/
2. From the host component APIs (which tell us the host on which the current component is resides)
http://:8080/api/v1/clusters//hosts//host_components/
Note that, we are referring both 1and 2 using the component names
But in the multi-component world for Ambari, we will come up with a situation like this, as shown below:
Core -> HDFS -> HDFS_CLIENT
edw -> HDFS -> HDFS_CLIENT
There is no good way to distinguish a given host component in A and B, if we continue to refer them with the component name end point (No. 2 API call).
However, No. 1 call is still fine as we will not allow 2 names for the same component within a given service, thus making it unique and allowing us to continue using the component names endpoint.
WORK DONE
In order to support multi-instance for components of a given service, we need to enhance the Host Components API so that they can distinguish one component instance of a service from another component instance of a service with the same Name.
The way to achieve this is to move Host Components API to be ID (number) based end point, compared to earlier being a name based endpoint. This will allow us to distinguish Core -> HDFS -> HDFS_CLIENT from edw -> HDFS -> HDFS_CLIENT, as an example.
Thus, following changes are required:
New field : component_type (eg: HDFS_CLIENT, HIVE_SERVER, ZOOKEEPER_SERVER etc)
Existing field : component_name will now hold the actual name given for the component at the time of install. For example:
A way to identify the HOST component uniquely via API calls, by way of referring them by ID instead of name.
MODIFIED API CALLs:
CHANGE : New field component_type
POST http://{{AmbariServer}}:8080/api/v1/clusters//servicegroups/core/services/HDFS/components
Body :
Response:
Body
{"HostRoles":{ "component_name":"HDFS_CLIENT2", "component_type":"HDFS_CLIENT", "service_group_name": "core", "service_name": "HDFS"}}
GET SERVICE COMPONENT:
CHANGE: host_components sub-resource will be referenced with end point as ID one.
GET http://{{AmbariServer}}:8080/api/v1/clusters//servicegroups/core/services/HDFS/components/HDFS_CLIENT2
Response:
GET HOST COMPONENT:
CHANGE: Called with ID based end point
GET http://{{AmbariServer}}:8080/api/v1/clusters//hosts//host_components/51
Response:
UPDATE SERVICE COMPONENTS
UPDATE HOST COMPONENTS
DELETE SERVICE COMPONENT:
DELETE HOST COMPONENT:
How was this patch tested?
Tested on live cluster, Details given above.