SPOTAUT-9153 Implementation of EMR Scaler Routes#133
Conversation
| * | ||
| * @return a list of Scaler cluster | ||
| */ | ||
| public List<ApiMrScalerListScalersAws> listMrScalers(ApiMrScalerListMrScalersRequest mrScalerListScalersRequest) { |
There was a problem hiding this comment.
Do we need the ApiMrScalerListMrScalersRequest object. We can directly pass the MrScalerId.
| * | ||
| * @return a list of instances | ||
| */ | ||
| public List<ApiMrScalerScaleUpAws> scaleUpMrScaler(ApiMrScalerScaleUpRequest mrScalerScaleUpRequest) { |
There was a problem hiding this comment.
Do we need the ApiMrScalerScaleUpRequest object. We can directly pass the MrScalerId and adjustment
| * | ||
| * @return a list of instances | ||
| */ | ||
| public List<ApiMrScalerScaleDownAws> scaleDownMrScaler(ApiMrScalerScaleDownRequest mrScalerScaleDownRequest) { |
There was a problem hiding this comment.
Same comment as previous function.
| * | ||
| * @return a list of instances | ||
| */ | ||
| public List<ApiMrScalerListInstancesAws> listMrScalerInstances( |
There was a problem hiding this comment.
This should return BL object list
| * | ||
| * @return a list of instances | ||
| */ | ||
| public List<ApiMrScalerListInstancesAws> listMrScalerInstances( |
There was a problem hiding this comment.
Client class shouldn't have any reference of Api level objects.
| * @return a list of Scaler cluster | ||
| */ | ||
| public List<ApiMrScalerListScalersAws> listMrScalers(ApiMrScalerListMrScalersRequest mrScalerListScalersRequest) { | ||
| List<ApiMrScalerListScalersAws> retVal; |
There was a problem hiding this comment.
Client class shouldn't have any reference of Api level objects.
| public List<ApiMrScalerListScalersAws> listMrScalers(ApiMrScalerListMrScalersRequest mrScalerListScalersRequest) { | ||
| List<ApiMrScalerListScalersAws> retVal; | ||
| String clusterToGet = mrScalerListScalersRequest.getMrScalerId(); | ||
| RepoGenericResponse<List<ApiMrScalerListScalersAws>> mrScalerListScalers = getSpotinstMrScalerListScalersRepo().listMrScalers(clusterToGet, authToken, account); |
There was a problem hiding this comment.
Client class shouldn't have any reference of Api level objects.
| * | ||
| * @return a list of instances | ||
| */ | ||
| public List<ApiMrScalerScaleDownAws> scaleDownMrScaler(ApiMrScalerScaleDownRequest mrScalerScaleDownRequest) { |
There was a problem hiding this comment.
Client class shouldn't have any reference of Api level objects.
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class BlMrScalerListInstancesAws { |
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class BlMrScalerListScalersAws { |
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class BlMrScalerScaleDownAws { |
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class BlMrScalerScaleUpAws { |
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerListInstancesAws; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.BlMrScalerListInstancesAws; | ||
|
|
||
| public class MrScalerListInstancesConverter { |
There was a problem hiding this comment.
Can we combine all 8 converter functions to single file/class.
| public class SpotinstMrScalerListInstancesRepo implements ISpotinstMrScalerListInstancesRepo { | ||
|
|
||
| @Override | ||
| public RepoGenericResponse<List<ApiMrScalerListInstancesAws>> listMrScalerInstances(String mrScalerId, String authToken, String account) { |
There was a problem hiding this comment.
Repo classes should return bl object list
| public class SpotinstMrScalerListScalersRepo implements ISpotinstMrScalerListScalersRepo { | ||
|
|
||
| @Override | ||
| public RepoGenericResponse<List<ApiMrScalerListScalersAws>> listMrScalers(String mrScalerId, String authToken, String account) { |
There was a problem hiding this comment.
Repo classes should return bl object list
| public class SpotinstMrScalerScaleDownRepo implements ISpotinstMrScalerScaleDownRepo { | ||
|
|
||
| @Override | ||
| public RepoGenericResponse<List<ApiMrScalerScaleDownAws>> scaleDownMrScaler(String mrScalerId, Integer adjustment, String authToken, String account) { |
There was a problem hiding this comment.
Repo classes should return bl object list
| public class SpotinstMrScalerScaleUpRepo implements ISpotinstMrScalerScaleUpRepo { | ||
|
|
||
| @Override | ||
| public RepoGenericResponse<List<ApiMrScalerScaleUpAws>> scaleUpMrScaler(String mrScalerId, Integer adjustment, String authToken, String account) { |
There was a problem hiding this comment.
Repo classes should return bl object list
|
|
||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerListInstancesAws; | ||
|
|
||
| public class ApiMrScalerListInstancesRequest { |
There was a problem hiding this comment.
Seems like this class is not required.
|
|
||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerListScalersAws; | ||
|
|
||
| public class ApiMrScalerListMrScalersRequest { |
There was a problem hiding this comment.
Seems like this class is not required.
|
|
||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerScaleDownAws; | ||
|
|
||
| public class ApiMrScalerScaleDownRequest { |
There was a problem hiding this comment.
Seems like this class is not required. This can be passed as parameters itself.
|
|
||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerScaleUpAws; | ||
|
|
||
| public class ApiMrScalerScaleUpRequest { |
There was a problem hiding this comment.
Seems like this class is not required. This can be passed as parameters itself.
| import com.spotinst.sdkjava.client.response.BaseServiceItemsResponse; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.BlMrScalerListInstancesAws; | ||
|
|
||
| public class ApiMrScalerListInstancesResponse extends BaseServiceItemsResponse<BlMrScalerListInstancesAws> { |
There was a problem hiding this comment.
Seems incorrect Bl object is extended to Api response.
| import com.spotinst.sdkjava.client.response.BaseServiceItemsResponse; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.BlMrScalerListScalersAws; | ||
|
|
||
| public class ApiMrScalerListMrScalersResponse extends BaseServiceItemsResponse<BlMrScalerListScalersAws> { |
There was a problem hiding this comment.
Seems incorrect Bl object is extended to Api response.
| import com.spotinst.sdkjava.client.response.BaseServiceItemsResponse; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.BlMrScalerScaleDownAws; | ||
|
|
||
| public class ApiMrScalerScaleDownResponse extends BaseServiceItemsResponse<BlMrScalerScaleDownAws> { |
There was a problem hiding this comment.
Seems incorrect Bl object is extended to Api response.
| import com.spotinst.sdkjava.client.response.BaseServiceItemsResponse; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.BlMrScalerScaleUpAws; | ||
|
|
||
| public class ApiMrScalerScaleUpResponse extends BaseServiceItemsResponse<BlMrScalerScaleUpAws> { |
There was a problem hiding this comment.
Seems incorrect Bl object is extended to Api response.
| import java.util.Map; | ||
|
|
||
| public class SpotinstMrScalerListInstancesService extends BaseSpotinstService { | ||
| public static List<BlMrScalerListInstancesAws> listMrScalerInstances(String clusterId, String authToken, String account) |
There was a problem hiding this comment.
Should return api object list
Don;t need 4 separate classes, this can be incorporated in the existing service class for MrScalers
SDK Changes
null check update
janetlinkiruba
left a comment
janetlinkiruba
left a comment
There was a problem hiding this comment.
please fix the comments
|
|
||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerAws; | ||
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerAwsCreationRequest; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.MrScalerAws; |
There was a problem hiding this comment.
is this class required now?
| package com.spotinst.sdkjava.model.service.mrScaler.aws; | ||
|
|
||
| import com.spotinst.sdkjava.client.response.BaseSpotinstService; | ||
| import com.spotinst.sdkjava.client.rest.RestClient; |
There was a problem hiding this comment.
All the MrScaler routes can be in same service class right?
| BlMrScalerOperatorResponse retVal = null; | ||
| public static ApiMrScalerOperatorAwsResponse createMrScalerOperator(MrScalerOperatorAws mrScalerOperatorAws, | ||
| String authToken, | ||
| String account) throws SpotinstHttpException { |
There was a problem hiding this comment.
For MrScalerOperator keep one service class
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.MrScalerOperatorAws; | ||
|
|
||
| public class MrScalerOperatorAwsRequest { | ||
| // @JsonProperty("mrScalerOperator") |
There was a problem hiding this comment.
remove this commented line
There was a problem hiding this comment.
Do we need this class?
There was a problem hiding this comment.
This can be passed directly without request class
|
|
||
| public class MrScalerListMrScalersRequest { | ||
| //region Members | ||
| private String mrScalerId; |
There was a problem hiding this comment.
why we need this request class
There was a problem hiding this comment.
This can be passed directly without request class
| import com.spotinst.sdkjava.model.api.mrScaler.aws.ApiMrScalerOperatorAwsResponse; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.MrScalerOperatorResponse; | ||
|
|
||
| public class SpotinstMrScalerAwsOperatorRepo implements ISpotinstMrScalerOperatorAwsRepo { |
There was a problem hiding this comment.
we can club all the functions into one repo class for MrScalerOperator
| private String id; | ||
| private String availabilityZone; | ||
| private String state; | ||
| private Date createdAt; |
| private List<String> instanceTypes; | ||
| private ElastigroupCapacityConfiguration capacity; | ||
| private String lifeCycle; | ||
| private MrScalerAwsEbsConfiguration ebsConfiguration; |
| * Created by aharontwizer on 7/27/15. | ||
| */ | ||
| class SpotinstElastigroupService extends BaseSpotinstService { | ||
| public class SpotinstElastigroupService extends BaseSpotinstService { |
There was a problem hiding this comment.
why is this class changed to public
| package com.spotinst.sdkjava.model; | ||
|
|
||
| import com.spotinst.sdkjava.exception.SpotinstNotSupportedException; | ||
| import com.spotinst.sdkjava.model.bl.mrScaler.aws.MrScalerScaleUpAws; |
There was a problem hiding this comment.
Not required to be separate interface for each function
4 participants
This is the default pull request template. You can customize it by adding a
pull_request_template.mdat the root of your repo or inside the.githubfolder.Jira Ticket
Include a link to your Jira Ticket
Example: JIRAISS-1234
Demo
Please add a recording of the feature/bug fix in work. if you added new routes, the recording should show the request and response for each new/changed route
Checklist: