Hi Lahiru,
On Tue, Apr 28, 2015 at 3:50 PM, Lahiru Sandaruwan <lahi...@wso2.com> wrote:
> 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.
>
IMHO we should expose whatever information we can from the Rest API, since
its the interaction point with Stratos. In a scenario where a third party
would need to integrate with Stratos via the Rest API it will be helpful if
Rest API provides such detailed information. Even when a user is using the
CLI or the UI, IMHO its meaningful to let the user know what exactly went
wrong. Just my two cents. If the community agrees, I'm ok with using 500
Internal Server error here.
>
> 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
>
> --
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> Thanks and Regards,
>
> Isuru H.
> <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>
> +94 716 358 048 <http://lk.linkedin.com/pub/lahiru-sandaruwan/16/153/146>*
> <http://wso2.com/>*
>
>
> * <http://wso2.com/>*
>
>
>
