You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Aidan Skinner <ai...@apache.org> on 2009/11/10 04:03:34 UTC
Remove printStackTrace from client and common
QPID-2192 is a bug a user raised about the random printStackTrace
calls that are littered about the place. This patch deletes all of the
ones in java/client and java/common I found that they might run into.
I'll commit this on Wednesday if nobody objects. The tests all pass
and they're fairly clearly wrong AFAICT.
- Aidan
--
Apache Qpid - AMQP, JMS, other messaging love http://qpid.apache.org
"A witty saying proves nothing" - Voltaire
Re: Remove printStackTrace from client and common
Posted by Rajith Attapattu <ra...@gmail.com>.
Looks fine to me.
Rajith
On Tue, Nov 10, 2009 at 8:13 AM, Aidan Skinner <ai...@apache.org> wrote:
> On Tue, Nov 10, 2009 at 3:40 AM, Robert Godfrey <ro...@gmail.com> wrote:
>
>> Hi Aidan,
>>
>> Why did you remove the setting of the linked exception here:
>
> Slippy fingers and tired eyes while looking at the diff. I'll put it back. :)
>
>> Also, we probably want to communicate the stack trace somehow in
>>
>> ---
>> a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
>> +++
>> b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
>> @@ -178,7 +178,6 @@ public class AMQProtocolHandlerTest extends TestCase
>> }
>> catch (Exception e)
>> {
>> - e.printStackTrace();
>> fail(e.getMessage());
>> }
>> }
>>
>> although I would think that just not catching the exception here would
>> probably do it...
>
> Yeah, not catching is probably the right thing to do there.
>
> - Aidan
> --
> Apache Qpid - AMQP, JMS, other messaging love http://qpid.apache.org
> "A witty saying proves nothing" - Voltaire
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
>
--
Regards,
Rajith Attapattu
Red Hat
http://rajith.2rlabs.com/
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Remove printStackTrace from client and common
Posted by Aidan Skinner <ai...@apache.org>.
On Tue, Nov 10, 2009 at 3:40 AM, Robert Godfrey <ro...@gmail.com> wrote:
> Hi Aidan,
>
> Why did you remove the setting of the linked exception here:
Slippy fingers and tired eyes while looking at the diff. I'll put it back. :)
> Also, we probably want to communicate the stack trace somehow in
>
> ---
> a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> +++
> b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> @@ -178,7 +178,6 @@ public class AMQProtocolHandlerTest extends TestCase
> }
> catch (Exception e)
> {
> - e.printStackTrace();
> fail(e.getMessage());
> }
> }
>
> although I would think that just not catching the exception here would
> probably do it...
Yeah, not catching is probably the right thing to do there.
- Aidan
--
Apache Qpid - AMQP, JMS, other messaging love http://qpid.apache.org
"A witty saying proves nothing" - Voltaire
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Remove printStackTrace from client and common
Posted by Robert Godfrey <ro...@gmail.com>.
2009/11/10 Martin Ritchie <ri...@apache.org>
> 2009/11/10 Robert Godfrey <ro...@gmail.com>:
> > Hi Aidan,
> >
> > Why did you remove the setting of the linked exception here:
> >
> > ---
> >
> a/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
> > +++
> >
> b/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
> > @@ -180,8 +180,6 @@ public class BasicMessageProducer_0_10 extends
> > BasicMessageProducer
> > catch (RuntimeException rte)
> > {
> > JMSException ex = new JMSException("Exception when sending
> > message");
> > - rte.printStackTrace();
> > - ex.setLinkedException(rte);
> > throw ex;
> > }
> > }
> >
> > Also, we probably want to communicate the stack trace somehow in
> >
> > ---
> >
> a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> > +++
> >
> b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> > @@ -178,7 +178,6 @@ public class AMQProtocolHandlerTest extends TestCase
> > }
> > catch (Exception e)
> > {
> > - e.printStackTrace();
> > fail(e.getMessage());
> > }
> > }
> >
> > although I would think that just not catching the exception here would
> > probably do it...
> >
> > Don't immediately see any problems with the patch otherwise since no
> > information should be lost... (i.e. the stack should still print to a log
> > somewhere if desired I guess)
>
> Removing the stackTrace alone here(AMQProtocolHandlerTest.java) is not
> the solution. This is a test and as Rob points out the exception needs
> to be reported back. I believe I wrote the test I clearly wasn't paing
> attention as calling fail() on a different thread will not fail the
> test.
>
> I'd say either leave the stacktrace in on this test .. with a comment
> asking anyone seeing this to please raise a JIRA; or ensure the
> exception is correctly communicated to the main test thread and test
> failure occurs.
>
> Just my thoughts.
>
> Martin
>
OK - I'm guilty of only looking at the diff and not the context... I just
assumed if you could call fail then throwing the exception would be the
answer...
I do think that causing the test to fail in some way would be the answer...
-- Rob
Re: Remove printStackTrace from client and common
Posted by Martin Ritchie <ri...@apache.org>.
2009/11/10 Robert Godfrey <ro...@gmail.com>:
> Hi Aidan,
>
> Why did you remove the setting of the linked exception here:
>
> ---
> a/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
> +++
> b/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
> @@ -180,8 +180,6 @@ public class BasicMessageProducer_0_10 extends
> BasicMessageProducer
> catch (RuntimeException rte)
> {
> JMSException ex = new JMSException("Exception when sending
> message");
> - rte.printStackTrace();
> - ex.setLinkedException(rte);
> throw ex;
> }
> }
>
> Also, we probably want to communicate the stack trace somehow in
>
> ---
> a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> +++
> b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
> @@ -178,7 +178,6 @@ public class AMQProtocolHandlerTest extends TestCase
> }
> catch (Exception e)
> {
> - e.printStackTrace();
> fail(e.getMessage());
> }
> }
>
> although I would think that just not catching the exception here would
> probably do it...
>
> Don't immediately see any problems with the patch otherwise since no
> information should be lost... (i.e. the stack should still print to a log
> somewhere if desired I guess)
Removing the stackTrace alone here(AMQProtocolHandlerTest.java) is not
the solution. This is a test and as Rob points out the exception needs
to be reported back. I believe I wrote the test I clearly wasn't paing
attention as calling fail() on a different thread will not fail the
test.
I'd say either leave the stacktrace in on this test .. with a comment
asking anyone seeing this to please raise a JIRA; or ensure the
exception is correctly communicated to the main test thread and test
failure occurs.
Just my thoughts.
Martin
> -- Rob
>
> 2009/11/10 Aidan Skinner <ai...@apache.org>
>
>> QPID-2192 is a bug a user raised about the random printStackTrace
>> calls that are littered about the place. This patch deletes all of the
>> ones in java/client and java/common I found that they might run into.
>> I'll commit this on Wednesday if nobody objects. The tests all pass
>> and they're fairly clearly wrong AFAICT.
>>
>> - Aidan
>>
>> --
>> Apache Qpid - AMQP, JMS, other messaging love http://qpid.apache.org
>> "A witty saying proves nothing" - Voltaire
>>
>>
>> ---------------------------------------------------------------------
>> Apache Qpid - AMQP Messaging Implementation
>> Project: http://qpid.apache.org
>> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>>
>
--
Martin Ritchie
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Remove printStackTrace from client and common
Posted by Robert Godfrey <ro...@gmail.com>.
Hi Aidan,
Why did you remove the setting of the linked exception here:
---
a/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
+++
b/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
@@ -180,8 +180,6 @@ public class BasicMessageProducer_0_10 extends
BasicMessageProducer
catch (RuntimeException rte)
{
JMSException ex = new JMSException("Exception when sending
message");
- rte.printStackTrace();
- ex.setLinkedException(rte);
throw ex;
}
}
Also, we probably want to communicate the stack trace somehow in
---
a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
+++
b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java
@@ -178,7 +178,6 @@ public class AMQProtocolHandlerTest extends TestCase
}
catch (Exception e)
{
- e.printStackTrace();
fail(e.getMessage());
}
}
although I would think that just not catching the exception here would
probably do it...
Don't immediately see any problems with the patch otherwise since no
information should be lost... (i.e. the stack should still print to a log
somewhere if desired I guess)
-- Rob
2009/11/10 Aidan Skinner <ai...@apache.org>
> QPID-2192 is a bug a user raised about the random printStackTrace
> calls that are littered about the place. This patch deletes all of the
> ones in java/client and java/common I found that they might run into.
> I'll commit this on Wednesday if nobody objects. The tests all pass
> and they're fairly clearly wrong AFAICT.
>
> - Aidan
>
> --
> Apache Qpid - AMQP, JMS, other messaging love http://qpid.apache.org
> "A witty saying proves nothing" - Voltaire
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
Re: Remove printStackTrace from client and common
Posted by Rafael Schloming <ra...@redhat.com>.
Aidan Skinner wrote:
> QPID-2192 is a bug a user raised about the random printStackTrace
> calls that are littered about the place. This patch deletes all of the
> ones in java/client and java/common I found that they might run into.
> I'll commit this on Wednesday if nobody objects. The tests all pass
> and they're fairly clearly wrong AFAICT.
I would modify your patch to change the printlns to log statements when
setLinkedException is involved. IIRC, the stack trace of the linked
exception is not included in the stack trace of the wrapping exception
when it is reported.
--Rafael
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org