You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@reef.apache.org by Sergiy Matusevych <se...@gmail.com> on 2016/06/02 21:08:50 UTC
Re: [DISCUSS] Deprecate Exceptions.Throw?
I agree with Markus on this case.
Exceptions.Throw() mixes up several aspects of functionality (control flow
and logging), and Julia suggests to add even more stuff to it (statistics).
For me as a new C# API user it was not quite clear at the beginning why the
function exists in the first place.. I wondered - am I still allowed to use
the regular throw, or should I always use that function? Does the function
*always* throw or sometimes suppresses the exception? What logging level
does it use?
So, to me, it looks like a redundant and unnecessary abstraction.
Cheers,
Sergiy.
On Tue, May 31, 2016 at 2:16 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2016-05-31 10:16 AM, Julia Wang (QIUHE) wrote:
>
>> I understand most people don't like it because at this time, we
>> haven't really started to leverage logs to do something. Moving
>> forward, we will need to use log to get statics data for monitoring ,
>> diagnose, etc. Have a centralized the place would be convenient for
>> us to have a consistent logs.
>>
>
> I agree with the goals. But `Exceptions.Throw` isn't the best way to get
> there. It confuses the compiler (and reader) of our code with stuff like
>
> ```
> Exceptions.Throw(...);
> return null;
> ```
>
> Also, `Exceptions.Throw` messes up the stack trace and leads to situations
> where we log the same Exception more than once. This is very difficult for
> us to debug, compared to the rules we adopted for the Java code. If I
> recall correctly, those are:
>
> (1) Exceptions are only logged once: when caught and dealt with or
> when crashing the process.
> (2) Exceptions always contain a message.
> (3) When the catch() block throws an exception in its own right, it
> must wrap the inner exception.
> (4) Use specific exception classes, not generic ones like `Exception`
>
> Details are in REEF-864.
>
> Those rules were made for Java. But as far as I can tell, C# supports
> Exceptions at about the same level as Java.
>
> What are we missing if we adhere to the above rules? How is it better
> addressed with `Exceptions.Throw()` and why is that worth the awkward code?
>
> Markus
>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
We seem to have reached some consensus. Hence, I created
[REEF-1467](https://issues.apache.org/jira/browse/REEF-1467)
to track this.
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Andrew Chung <af...@gmail.com>.
I'm still +1 as well.
Thanks,
Andrew
On Wed, Jun 15, 2016 at 9:16 AM, Dhruv Mahajan <dh...@gmail.com> wrote:
> I am +1 on removing this right from the beginning and doing that now also.
> IT leads to weird warnings and extra return null statements that are not
> desirable.
>
> On Wed, Jun 15, 2016 at 8:43 AM, Markus Weimer <ma...@weimo.de> wrote:
>
>> Hi,
>>
>> bumping this to everyone's attention again. I still maintain that we
>> should not interfere with .NET's exception handling and use `throw` instead
>> of the methods in the `Exception` class.
>>
>> At the same time, we should have good coding standards around the use of
>> exceptions, for which REEF-864 can be the basis, unless where that
>> contradicts best practices for .NET.
>>
>> We should also strive to enforce such good practices via tools. I am not
>> sure whether the StyleCop and friends can do that. Does anyone know?
>>
>> Markus
>>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Dhruv Mahajan <dh...@gmail.com>.
I am +1 on removing this right from the beginning and doing that now also.
IT leads to weird warnings and extra return null statements that are not
desirable.
On Wed, Jun 15, 2016 at 8:43 AM, Markus Weimer <ma...@weimo.de> wrote:
> Hi,
>
> bumping this to everyone's attention again. I still maintain that we
> should not interfere with .NET's exception handling and use `throw` instead
> of the methods in the `Exception` class.
>
> At the same time, we should have good coding standards around the use of
> exceptions, for which REEF-864 can be the basis, unless where that
> contradicts best practices for .NET.
>
> We should also strive to enforce such good practices via tools. I am not
> sure whether the StyleCop and friends can do that. Does anyone know?
>
> Markus
>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
Hi,
bumping this to everyone's attention again. I still maintain that we
should not interfere with .NET's exception handling and use `throw`
instead of the methods in the `Exception` class.
At the same time, we should have good coding standards around the use of
exceptions, for which REEF-864 can be the basis, unless where that
contradicts best practices for .NET.
We should also strive to enforce such good practices via tools. I am not
sure whether the StyleCop and friends can do that. Does anyone know?
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
On 2016-06-07 3:29 PM, Dhruv Mahajan wrote:
> Just to be clear, are we saying that we are deprecating
> Exception.Throw and Exceptions.CatchAndThrow?
I am advocating to deprecate the whole class. Once we have consensus on
that.
I was specifically referring to the case you mentioned:
On 2016-06-07 2:43 PM, Dhruv Mahajan wrote:
> It is caught and thrown but the whole stack trace is lost. Can
> somebody confirm that? By changes in evaluator I meant that stack
> trace should also be shown.
This happens when the Evaluator catches an Exception and then exits,
right? Erasing the stack trace at that time is indeed bad form and
should be fixed. That is what I meant with catch and throw which should
be avoided.
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Dhruv Mahajan <dh...@gmail.com>.
Just to be clear, are we saying that we are deprecating Exception.Throw and
Exceptions.CatchAndThrow?
On Tue, Jun 7, 2016 at 2:49 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2016-06-07 2:43 PM, Dhruv Mahajan wrote:
>
>> It is caught and thrown but the whole stack trace is lost. Can somebody
>> confirm that? By changes in evaluator I meant that stack trace should also
>> be shown.
>>
>
> +1 on not doing a catch and rethrow. That is bad form. Can you file a JIRA
> on this?
>
> Markus
>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
On 2016-06-07 2:43 PM, Dhruv Mahajan wrote:
> It is caught and thrown but the whole stack trace is lost. Can somebody
> confirm that? By changes in evaluator I meant that stack trace should also
> be shown.
+1 on not doing a catch and rethrow. That is bad form. Can you file a
JIRA on this?
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Dhruv Mahajan <dh...@gmail.com>.
It is caught and thrown but the whole stack trace is lost. Can somebody
confirm that? By changes in evaluator I meant that stack trace should also
be shown. Sorry it was not clear.
Dhruv
On Tue, Jun 7, 2016 at 1:45 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2016-06-07 11:42 AM, Dhruv Mahajan wrote:
>
>> Moreover, it seems to me that in current evaluator, if u simply
>> throw the exception, it is caught, and a Level.Error message is
>> given. Am I right?
>>
>
> Any exception thrown but not caught should be caught by a process-level
> exception handler. That handler logs the exception and fails the
> process. If not, we have a bug.
>
> I guess this implies that we just cannot simply start using throw
>> but need some changes in evaluator also.
>>
>
> How do you mean?
>
> Markus
>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
On 2016-06-07 11:42 AM, Dhruv Mahajan wrote:
> Moreover, it seems to me that in current evaluator, if u simply
> throw the exception, it is caught, and a Level.Error message is
> given. Am I right?
Any exception thrown but not caught should be caught by a process-level
exception handler. That handler logs the exception and fails the
process. If not, we have a bug.
> I guess this implies that we just cannot simply start using throw
> but need some changes in evaluator also.
How do you mean?
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Dhruv Mahajan <dh...@gmail.com>.
Hi
So in REEF-1407 that I am handling currently, I have to lots of exception
handling. If we have decided to remove the Exceptions.* business I would
just use a simple throw. Moreover, it seems to me that in current
evaluator, if u simply throw the exception, it is caught, and a Level.Error
message is given. Am I right? I guess this implies that we just cannot
simply start using throw but need some changes in evaluator also.
Dhruv
On Mon, Jun 6, 2016 at 11:50 AM, Markus Weimer <ma...@weimo.de> wrote:
> On 2016-06-06 11:13 AM, Julia Wang (QIUHE) wrote:
>
>> An example that Beysim shows to me was a case where we throw
>> exception with no error message. If we used
>> Org.Apache.REEF.Utilities.Diagnostics. Exceptions. Throw() we would
>> have seen the error.
>>
>
> Yes, having no error message is bad form. Can we enforce that via
> something like checkstyle, but for C#?
>
> Markus
>
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
On 2016-06-06 11:13 AM, Julia Wang (QIUHE) wrote:
> An example that Beysim shows to me was a case where we throw
> exception with no error message. If we used
> Org.Apache.REEF.Utilities.Diagnostics. Exceptions. Throw() we would
> have seen the error.
Yes, having no error message is bad form. Can we enforce that via
something like checkstyle, but for C#?
Markus
RE: [DISCUSS] Deprecate Exceptions.Throw?
Posted by "Julia Wang (QIUHE)" <Qi...@microsoft.com>.
An example that Beysim shows to me was a case where we throw exception with no error message. If we used Org.Apache.REEF.Utilities.Diagnostics. Exceptions. Throw() we would have seen the error.
Julia
-----Original Message-----
From: Julia Wang (QIUHE)
Sent: Monday, June 6, 2016 10:53 AM
To: dev@reef.apache.org
Subject: RE: [DISCUSS] Deprecate Exceptions.Throw?
Let me check from Beysim. Original thought was from Beysim and Yinda's previous project. Personally I don't have strong preference as usually I like to use original .Net APIs rather than wrappers :).
-----Original Message-----
From: Markus Weimer [mailto:markus@weimo.de]
Sent: Monday, June 6, 2016 8:38 AM
To: dev@reef.apache.org
Subject: Re: [DISCUSS] Deprecate Exceptions.Throw?
So, what do we do about this? As it stands, we don't deprecate the class based on Julia's veto.
Markus
RE: [DISCUSS] Deprecate Exceptions.Throw?
Posted by "Julia Wang (QIUHE)" <Qi...@microsoft.com>.
Let me check from Beysim. Original thought was from Beysim and Yinda's previous project. Personally I don't have strong preference as usually I like to use original .Net APIs rather than wrappers :).
-----Original Message-----
From: Markus Weimer [mailto:markus@weimo.de]
Sent: Monday, June 6, 2016 8:38 AM
To: dev@reef.apache.org
Subject: Re: [DISCUSS] Deprecate Exceptions.Throw?
So, what do we do about this? As it stands, we don't deprecate the class based on Julia's veto.
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
So, what do we do about this? As it stands, we don't deprecate the class
based on Julia's veto.
Markus
Re: [DISCUSS] Deprecate Exceptions.Throw?
Posted by Markus Weimer <ma...@weimo.de>.
On 2016-06-02 2:22 PM, Julia Wang (QIUHE) wrote:
> The log level is always Error as it is throwing an exception .
That is part of the problem. In many cases, we write code that deals
with the exception, e.g. if it is caused by a network timeout, where we
just try again. In that case, the exception is not an ERROR. It might be
best to not log it at all, really.
I like the principles for exception handling that Sergey dug up for
Java: Exceptions should be logged exactly once, when they cause the
system to crash.
> I can only speak for the original purpose of this lib. The value was
> to have a consistent way to log error for analyzing logs in future.
I do not thing we have achieved that. We log exceptions more than once
in this way, conflate the control flow and potentially mess up
optimizations by the compiler. And that is just our code. We have no way
of nudging application code to use the same API.
That said, if there is something wrong with exception handling in .NET,
we should file a bug against it [0].
Markus
[0]: https://github.com/dotnet/coreclr/issues
RE: [DISCUSS] Deprecate Exceptions.Throw?
Posted by "Julia Wang (QIUHE)" <Qi...@microsoft.com>.
The log level is always Error as it is throwing an exception :).
When we developed this lib, we replaced everywhere in REEF code. Although we have no way to prevent developers from using .Net "throw".
For the issues pointed by Markus, we can always improve it. I guess the question is do we want to improve it or discard it.
I can only speak for the original purpose of this lib. The value was to have a consistent way to log error for analyzing logs in future.
Julia
-----Original Message-----
From: Sergiy Matusevych [mailto:sergiy.matusevych@gmail.com]
Sent: Thursday, June 2, 2016 2:09 PM
To: dev@reef.apache.org
Subject: Re: [DISCUSS] Deprecate Exceptions.Throw?
I agree with Markus on this case.
Exceptions.Throw() mixes up several aspects of functionality (control flow and logging), and Julia suggests to add even more stuff to it (statistics).
For me as a new C# API user it was not quite clear at the beginning why the function exists in the first place.. I wondered - am I still allowed to use the regular throw, or should I always use that function? Does the function
*always* throw or sometimes suppresses the exception? What logging level does it use?
So, to me, it looks like a redundant and unnecessary abstraction.
Cheers,
Sergiy.
On Tue, May 31, 2016 at 2:16 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2016-05-31 10:16 AM, Julia Wang (QIUHE) wrote:
>
>> I understand most people don't like it because at this time, we
>> haven't really started to leverage logs to do something. Moving
>> forward, we will need to use log to get statics data for monitoring ,
>> diagnose, etc. Have a centralized the place would be convenient for
>> us to have a consistent logs.
>>
>
> I agree with the goals. But `Exceptions.Throw` isn't the best way to
> get there. It confuses the compiler (and reader) of our code with
> stuff like
>
> ```
> Exceptions.Throw(...);
> return null;
> ```
>
> Also, `Exceptions.Throw` messes up the stack trace and leads to
> situations where we log the same Exception more than once. This is
> very difficult for us to debug, compared to the rules we adopted for
> the Java code. If I recall correctly, those are:
>
> (1) Exceptions are only logged once: when caught and dealt with or
> when crashing the process.
> (2) Exceptions always contain a message.
> (3) When the catch() block throws an exception in its own right, it
> must wrap the inner exception.
> (4) Use specific exception classes, not generic ones like
> `Exception`
>
> Details are in REEF-864.
>
> Those rules were made for Java. But as far as I can tell, C# supports
> Exceptions at about the same level as Java.
>
> What are we missing if we adhere to the above rules? How is it better
> addressed with `Exceptions.Throw()` and why is that worth the awkward code?
>
> Markus
>