On Nov 5, 2012, at 12:36 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
> Hi
> On 04/11/12 20:36, Marek Potociar wrote:
>>
>> On Nov 4, 2012, at 8:16 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>
>>> On 02/11/12 18:51, Marek Potociar wrote:
>>>>
>>>> On Nov 2, 2012, at 6:13 PM, Sergey Beryozkin<sberyozkin_at_talend.com> wrote:
>>>>
>>>>> Hi
>>>>> On 02/11/12 16:36, Marek Potociar wrote:
>>>>>> I'm still not sure what's the big picture in your proposal:
>>>>>>
>>>>>> - You indicated earlier that we should drop ClientException, now you're
>>>>>> adding new ClientException subtypes.
>>>>>> - ResponseProcessingException seems like something that should be better
>>>>>> derived from MessageProcessingException, but you're again using
>>>>>> ClientException
>>>>>>
>>>>>> Also, why should we create new connection-related exceptions if Java SE
>>>>>> already providers a hierarchy of IOException subclasses in java.net
>>>>>> <http://java.net> package that cover these failures (ConnectException,
>>>>>> UnknownHostException, NoRouteToHostException, HttpRetryException...)?
>>>>>>
>>>>>> It seems to me that, if we really want to do anything about improving
>>>>>> the current processing exception hierarchy, we could do e.g. this:
>>>>>>
>>>>>> 1. Drop ClientException
>>>>>> 2. Define a common ProcessingException instead and update client APIs
>>>>>> to throw ProcessingException
>>>>>> * On client side this exception would wrap any exceptions not
>>>>>> related to message processing in filters or interceptors,
>>>>>> including IOExceptions thrown by the transport layer or MBR/MBW.
>>>>>> 3. Make MessageProcessingException a subclass of ProcessingException
>>>>>> 4. Add new client-side ResponseProcessingException subclass of
>>>>>> MessageProcessingException
>>>>>>
>>>>>> I know we did not get rid of getCause() in case of IOExceptions, but I
>>>>>> it still seems to address most of the other issues you had (wrapping
>>>>>> filter/interceptor exceptions, missing response processing exception).
>>>>>> Would that work for you? Sergey, others would that work for you too?
>>>>>>
>>>>>
>>>>> I just like the expression "ClientException". What does this proposal improve on the server side of things ? I think I'm more comfortable with the latest proposal from Bill, but I'd like a single catch for "ClientException" be sufficient, at the bare level, with respect to managing client-centric exception of all sorts
>>>>
>>>> You could have a single try-catch for a ProcessingException. That would fully cover what ClientException covers and additionally remove the need to invoke getCause for addressing message processing exception sub-types. Additionally, that would also cover exception thrown from Response.readEntity().
>>>>
>>>> Isn't that worth considering?
>>>>
>>>
>>> Sounds like a good idea. I'm just sensing something is not right. Purely refactoring wise it is spot on. I think there has to be construct there dedicated to representing "client" centric exceptions.
>>> We are going to lose it...
>>
>> I think I know what you mean - I had the same feeling. But facing the facts I could not find a valid reason why the above would not work. The only problematic use case I could think of is covered by a client-specific ResponseProcessingException. So I really don't could not find a solid argument why the above would not work. Plus, the hierarchy really seems to make a lot of sense.
>>
>
> And how we'd represent client-side exceptions to do with the connection failures ? It is not a ProcessingException.
>
> With ClientException one can consider creating ConnectionTimeoutException, etc.
I thought that is explained in the proposal. Connection exceptions represented as IOException sub-classes would be wrapped in the processing exception. Any processing exception thrown on the client side (other than MessageProcessingException) so far typically means it's somehow related to connection errors.
>
> It appears the refactoring is proposed is mostly because ClientException.getCause() is not considered a good programming practice...
Actually, it's not true at all. Connections errors are proposed to still be accessed via ProcessingException.getCause().
> May be in some cases that is the case. But I disagree with this opinion leading to the refactoring which 'loses' the client-centric exception class.
>
> Exception.getCause() is there for a reason, it is when the underlying exception has to be visible to the catch block.
>
> When we do "client.get(Book.class)" is it that case exactly, as opposed to "Response r = client.get()". I can see why the refactoring can make ClientException.getCause() not necessary, but it really has its cons too.
>
> As far as various compromises are concerned, IMHO the current approach wins
I don't see what is the big difference between checking for ProcessingException and checking for ClientException. If are so strongly convinced that people need to process client-side connection exceptions, then we need to seriously consider throwing a checked IOException from our client APIs.
Btw. other than ranting, I did not see any real reasoning in your email when it comes to the question why we need to have a client-side specific root exception for processing. Also, the current approach is not a compromise, or at least it was not designed as a result of a compromise. Similarly, I'm not positioning the proposed new approach as a compromise just because it satisfies some of the requirements of each opposing side. I'm proposing it as I think it is better in terms of present reusability and future extensibility. The compromise is in finding the line between the present and future extensions. (IOW, should we find out we need more client-specific exceptions, we can add them later. The common ProcessingException does the same job as a ClientException would do in that use case. (IMHO)
Marek
>
> Thanks, Sergey
>
>
>
>
>
>
>
>> Marek
>>
>>>
>>> Sergey
>>>
>>>> Marek
>>>>
>>>>>
>>>>> Or just leave things as they are now, I know Bill is not comfortable with ClientException.getCause() but given Response.readEntity(MyClass.class) will throw MessageProcessingException, clientException.getCause() in case of myClient.get(MyClass.class) seems if not perfect but reasonable IMHO
>>>>>
>>>>> Sergey
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Marek
>>>>>>
>>>>>> On Nov 2, 2012, at 4:06 PM, Bill Burke<bburke_at_redhat.com
>>>>>> <mailto:bburke_at_redhat.com>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 11/2/2012 10:28 AM, Marek Potociar wrote:
>>>>>>>> Bill,
>>>>>>>>
>>>>>>>> You lost me.
>>>>>>>>
>>>>>>>
>>>>>>> Eh, maybe some of my ideas aren't that thought out, but.... Consider
>>>>>>> this scenario:
>>>>>>>
>>>>>>> what do you do when there is response filter/interceptor/MBR failures?
>>>>>>> i.e.
>>>>>>>
>>>>>>> Order order = target.request().post(Order.class, entity);
>>>>>>>
>>>>>>> What if the server returns 200, and a response filter, interceptor or
>>>>>>> MBR fails? The client app might want to know a 200 was sent back. As,
>>>>>>> server-side, the operation is still successful.
>>>>>>>
>>>>>>> So don't we need an exception like this?
>>>>>>>
>>>>>>> // thrown if response filter or MBR or interceptor fails
>>>>>>> class ResponseProcessingException extends ClientException {
>>>>>>>
>>>>>>> Response getResponse() {...}
>>>>>>> }
>>>>>>>
>>>>>>> readEntity() should probably still throw MessageProcessingException
>>>>>>>
>>>>>>> Might also want to have:
>>>>>>>
>>>>>>> // can't send request because of bad IP address, can't connect, etc...
>>>>>>> class ClientConnectionFailure extends ClientException {}
>>>>>>>
>>>>>>> // thrown if socket times out on a request
>>>>>>> class ClientTimeout extends ClientException {}
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Bill Burke
>>>>>>> JBoss, a division of Red Hat
>>>>>>> http://bill.burkecentral.com
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Sergey Beryozkin
>>>
>>> Talend Community Coders
>>> http://coders.talend.com/
>>>
>>> Blog: http://sberyozkin.blogspot.com
>>