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
