Hi Isuru, Thanks for the input.
On Tue, Apr 28, 2015 at 3:41 PM, Isuru Haththotuwa <isu...@apache.org> wrote: > IMHO returning 500 internal server error here might not exactly convey the > problem. AFAIU, RemoteException is thrown when we can't connect to a remote > host, maybe due to a network issue, or remote host being offline, etc. It > might be more explanatory to return 504 (Gateway Timeout), WDYT? > Aggree that this would be giving better information. But from the Rest API user POV, this is an internal issue, even though it is a timeout/ network issue actually. IMO we do not need to give Autoscaler service or Cloud controller service issues(Network/ timeout) to the Rest API user. Thanks. > > On Tue, Apr 28, 2015 at 2:43 PM, Anuruddha Liyanarachchi < > anurudd...@wso2.com> wrote: > >> +1 for throwing *500 Internal Server Error *for all the RemoteException. >> >> >> >> On Tue, Apr 28, 2015 at 2:23 PM, Lahiru Sandaruwan <lahi...@wso2.com> >> wrote: >> >>> Hi, >>> >>> I have fixed few success codes, and looking into error codes now. For >>> all the "RemoteException"s thrown due to service unavailability etc. we >>> should through *500 Internal Server Error.* >>> >>> Wdyt? >>> >>> Thanks. >>> >>> On Sun, Apr 19, 2015 at 12:40 AM, Chamila De Alwis <chami...@wso2.com> >>> wrote: >>> >>>> Hi devs, >>>> >>>> After going through the REST API and the tickets that are raised for >>>> it, I think we should do another round of refactoring on it. Most API >>>> methods will stay unchanged, and there will be no additional methods added >>>> or existing methods removed. However there are some improvements that can >>>> be done throughout the REST API that can contribute to a better, more >>>> standardized API. Some of these suggestions have been under discussion for >>>> sometime, and this mail will serve as a place to consolidate them all. >>>> >>>> *1. Status Codes * >>>> There are a number of issues that are related to wrong or no status >>>> codes being returned from the API methods. For an example, most GET methods >>>> used to return 200 Ok status even for empty result sets. I have fixed most >>>> of the issues, however IMO if we can stick to a fixed, consistent, small >>>> range of status codes, that can greatly improve the usage experience for >>>> the API. >>>> >>>> Following are the status codes that we are returning now. >>>> >>>> >>>> - *200 Ok* - GET, DELETE, POST, PUT >>>> - *201 Created* - POST >>>> - *202 Accepted* - Used only twice in deployApplication and >>>> undeployApplication POST methods >>>> - *209 No Content* - Once in a DELETE method, >>>> removeKubernetesHostCluster >>>> - *404 Not Found *- GET, POST, PUT, DELETE >>>> - *409 Conflict* - POST, PUT >>>> - *400 Bad Request* - for RestApiException which is used frequently >>>> losing the meaning of the status code >>>> - *500 Internal Server Error* >>>> >>>> >>>> We can improve this range by shortening down the status codes being >>>> sent back, and avoiding using invalid status codes for certain operations. >>>> What I'm proposing looks something like this. >>>> >>>> >>>> - *200 Ok* - GET, DELETE (with content body), PUT - Success >>>> - *201 Created* - POST - Resource Created, send Location header >>>> - *404 Not Found* - GET, POST, PUT, DELETE >>>> - *409 Conflict* - POST, PUT - Resource being added already exists >>>> - *400 Bad Request* - Other request error (only) >>>> - *500 Internal Server Error* - Error is on our side (which most >>>> catch clauses should go to) >>>> >>>> >>>> IMO we can drop *202* and *209* status codes as being too granular. We >>>> are always returning a content body for every DELETE request being done. >>>> Therefore a client expecting a *209 No Content *status code would not >>>> make sense [1]. The case for *202 Accepted* is there, however we can >>>> reduce our spectrum of status codes to expect for a client, by returning >>>> *201 >>>> Created* for deploy/undeploy operations as well. The status of the >>>> application deployment will anyway will be queried by *GET >>>> "/applications/app-id/runtime"*, so we can use 201 Created without an >>>> issue. Furthermore we should stop sending 200 Ok for successful POST >>>> operations. >>>> >>>> *2. Operation Types* >>>> Following are the PUT operations that IMO need to be improved. >>>> >>>> Method NameResource PathSuggested ImprovementaddKubernetesHostPUT >>>> "/kubernetesClusters/{kubernetesClusterId}/minion"*POST* >>>> "/kubernetesClusters/{kubernetesClusterId}/minion"updateKubernetesHostPUT >>>> "/kubernetes/update/host"PUT " >>>> */kubernetesClusters/{kubernetesClusterId}/minion/{minionId}*" >>>> updateDeploymentPolicyPUT "/deploymentPolicies"PUT "/deploymentPolicies >>>> */{deploymentPolicyId}*"updateCartridgePUT "/cartridges"PUT >>>> "/cartridges*/{cartridgeId}*"updateApplicationPolicyPUT >>>> "/applicationPolicies"PUT "/applicationPolicies*/{applicationPolicyId}* >>>> "updateAutoscalingPolicyPUT "/autoscalingPolicies"PUT >>>> "/autoscalingPolicies*/{autoscalingPolicyId}*"updateNetworkPartitionPUT >>>> "/networkPartitions"PUT "/networkPartitions*/{networkPartitionId}*" >>>> updateTenantPUT "/tenants"PUT "/tenants*/{tenantId}*"updateUserPUT >>>> "/users"PUT "/users*/{userId}*" >>>> >>>> PUT operations should point to the exact resource location where as >>>> POST operations do not have to [2]. The other alternative to the >>>> improvements suggested above is to handle updates for them in the POST >>>> operation itself, however it make things more complex for a client using >>>> the API. >>>> >>>> *3. Exception Throwing* >>>> Currently we are throwing RestAPIExceptions almost every time a server >>>> side exception is caught. This results in *400 Bad Request* being >>>> returned for requests that might actually be correct. It should only be >>>> thrown for verified bad requests. For this to take place there should be >>>> validations for requests and custom exceptions from the server side that >>>> reflect the error status in a granular manner. As a side note to this, as >>>> I've observed, currently no custom exception from the Autoscaler component >>>> is correctly deserialized and mapped at the stub and therefore only an >>>> AxisFault is thrown from the server. This should be investigated. >>>> >>>> *4. Response structure* >>>> Currently there are two bean classes for success and error responses >>>> (SuccessResponseBean and ErrorResponseBean). From the Jaggery code, based >>>> on the status code range, the successMessage or errorMessage is printed. >>>> The status code is also sent as statusCode (SuccessResponseBean) and >>>> errorCode (ErrorResponseBean). IMO this can be unified in to one response >>>> bean. Furthermore, the response should not contain two status codes, which >>>> can result in frequent different status codes being returned for one >>>> request. Since the API is mostly returning consistent status codes, the use >>>> of two status codes can be avoided. >>>> >>>> *{"statusCode":202,"successMessage":"Application deployed successfully: >>>> [application] complex-app"}* >>>> >>>> >>>> >>>> These are the improvements that I was able to recognize by going >>>> through the code. If you have any input on these please feel free to add. >>>> Although some of these can be pushed for a later release, IMO others would >>>> greatly improve the API status for the next release. >>>> >>>> [1] - http://tools.ietf.org/html/rfc7231#section-4.3.5 >>>> [2] - http://restcookbook.com/HTTP%20Methods/put-vs-post/ >>>> >>>> Regards, >>>> Chamila de Alwis >>>> Software Engineer | WSO2 | +94772207163 >>>> Blog: code.chamiladealwis.com >>>> >>>> >>>> >>> >>> >>> -- >>> -- >>> Lahiru Sandaruwan >>> Committer and PMC member, Apache Stratos, >>> Senior Software Engineer, >>> WSO2 Inc., http://wso2.com >>> lean.enterprise.middleware >>> >>> phone: +94773325954 >>> email: lahi...@wso2.com blog: http://lahiruwrites.blogspot.com/ >>> linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146 >>> >>> >> >> >> -- >> *Thanks and Regards,* >> Anuruddha Lanka Liyanarachchi >> Software Engineer - WSO2 >> Mobile : +94 (0) 712762611 >> Tel : +94 112 145 345 >> a <thili...@wso2.com>nurudd...@wso2.com >> >> -- >> <nurudd...@wso2.com> >> <nurudd...@wso2.com> >> Thanks and Regards, >> >> Isuru H. >> <nurudd...@wso2.com> >> +94 716 358 048 <nurudd...@wso2.com>* <http://wso2.com/>* >> >> >> * <http://wso2.com/>* >> >> >> -- -- Lahiru Sandaruwan Committer and PMC member, Apache Stratos, Senior Software Engineer, WSO2 Inc., http://wso2.com lean.enterprise.middleware phone: +94773325954 email: lahi...@wso2.com blog: http://lahiruwrites.blogspot.com/ linked-in: http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146