[
https://issues.apache.org/jira/browse/THRIFT-5333?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17258715#comment-17258715
]
Denys Rtveliashvili commented on THRIFT-5333:
---------------------------------------------
Yes, as the IDL-defined exceptions are inheriting from TException you _can_
handle them any way you wish. However, that does not mean this is convenient to
handle them or that the handling is error-prone.
The current approach leads to a situation that the semantic difference between
exceptions defined in IDL and exceptions internal to the workings of Thrift
itself is completely erased.
This is how it impacts in practice.
Say, I am writing a code which calls _myMethod_. I write a call
"rpc.myMethod(12345)" and I see that exceptions are being thrown.
If I use IDE of some sort, it will likely offer me to wrap that with try-catch
and the only catch close would likely be the one for TException. The other
exception (MyException) will be ignored as it is covered by TException clause.
Even if I do write an additional clause specifically for that exception and do
the appropriate handling, soon enough the code may go rotten because if another
exception is added in IDL then the old code would still compile while it would
be totally blind to the newly added exception.
Another situation is where one may want to catch specifically Thrift-internal
exceptions but let the IDL-defined ones propagate further.
One can write something like this:
{code:java}
try {
rpc.myMethod(12345);
} catch (MyException e) {
throw e;
} catch (TException e) {
// do something about the thrift-internal matters
}{code}
This is OK, if more exceptions are added, one has to add them to the list of
re-throwable ones and a catch clause for TException will effectively mask the
addition of new ones as IDL evolves.
On the other hand, if those IDL-exceptions are not extending TException, then
the code would look like this instead:
{code:java}
try {
rpc.myMethod(12345);
} catch (TException e) {
// do something about the thrift-internal matters
}
{code}
This is less code and importantly, the result of adding a new exception to IDL
would result in compiler spotting that another exception needs to be added to
"throws" list of the method or handled in try-catch.
On the other hand, I see your point that this is how it works now and the
change is not backwards compatible.
Perhaps it can be made into a switch for Java compiler?
For the time being what I do is this: I run a compiler and then manually tweak
signatures of all exceptions in the autogenerated code. While being an
unnatural thing to do, it allows me to uncover potential bugs in the code.
> Exceptions defined in IDL to extend Exception rather than TException
> --------------------------------------------------------------------
>
> Key: THRIFT-5333
> URL: https://issues.apache.org/jira/browse/THRIFT-5333
> Project: Thrift
> Issue Type: Improvement
> Components: Java - Compiler
> Affects Versions: 0.13.0
> Reporter: Denys Rtveliashvili
> Priority: Major
>
> At the moment, if an exception is defined in IDL and Java code is generated
> for it, the exception would be defined as extending TException like this:
> {code:java}
> public class MyException extends org.apache.thrift.TException implements ...
> {code}
> At the same time, definition of methods looks like this:
> {code:java}
> public int myMethod(int argument) throws MyException,
> org.apache.thrift.TException {
> ...
> }{code}
> Here, TException has to be included as that is the only way to signal that
> RPC call has failed for whatever reason.
> Now the problem with this is that if one calls a method like that, it is
> obvious that TException covers all those exceptions defined in IDL and so
> there is no way to clearly see which exceptions are result of internal
> workings of Thrift and which ones correspond to situations described in IDL:
> {code:java}
> try {
> myMethod(123);
> } catch (TException e) {
> log.error("Problem spotted", e);
> }{code}
>
> Why not make extensions extend java.lang.Exception instead?
> {code:java}
> public class MyException extends java.lang.Exception implements ...{code}
> This would have made it clear which exceptions are throwable by the call as
> "catch (TException e)" would not catch them and explicit catching would be in
> order:
> {code:java}
> try {
> myMethod(123);
> } catch (MyException e) {
> log.error("MyException has been thrown");
> } catch (TException e) {
> log.error("Problem spotted", e);
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
