qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: QPID-3233 - In some cases the JMS Session does not get notified of the AMQP session closed, due to an execution exception received from the broker.
Date Mon, 02 May 2011 16:43:54 GMT


> On 2011-04-29 14:51:54, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
line 580
> > <https://reviews.apache.org/r/676/diff/1/?file=17690#file17690line580>
> >
> >     There are several tabs introduced by the patch here, they should be changed
to spaces.
> >     
> >     Should we really replace the IllegalStateException already being thrown previously?
The previous behaviour was to chain the additional exception to it.

The previously thrown IllegalStateException just contained an error message that didn't really
give much information.
I wanted to include the "Error code" in the message of the main exception which makes it easy
for people looking at logs.


> On 2011-04-29 14:51:54, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
line 718
> > <https://reviews.apache.org/r/676/diff/1/?file=17690#file17690line718>
> >
> >     If closed() were used for the task instead of this method then it could remain
private.

I think "closed" is probably the correct method to use in this case. 
I will change accordingly.


> On 2011-04-29 14:51:54, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 920
> > <https://reviews.apache.org/r/676/diff/1/?file=17691#file17691line920>
> >
> >     There are several tabs introduced by the patch, they should be changed to spaces.

I will fix this, I have the settings to convert tabs to spaces, not sure what happened.


> On 2011-04-29 14:51:54, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 924
> > <https://reviews.apache.org/r/676/diff/1/?file=17691#file17691line924>
> >
> >     Should we not be using AMQSession.closed() as the super implementation, rather
than the close() method?

Agreed as I mentioned in my previous comment.


> On 2011-04-29 14:51:54, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java,
line 588
> > <https://reviews.apache.org/r/676/diff/1/?file=17692#file17692line588>
> >
> >     There are several tabs introduced by the patch here, they should be changed
to spaces.
> >     
> >     The code would probably look cleaner and potentially be more accurate by calling
manager.getLastException() once upfront and using the result for all further processing rather
than calling the method up to 3 times.

I thought about it, but decided for the purpose of this patch to leave existing code as it
is to better highlight what was added and what was merely re-arranged.
For the final commit I will definitely do what you suggested.


- rajith


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/676/#review616
-----------------------------------------------------------


On 2011-04-29 03:15:23, rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/676/
> -----------------------------------------------------------
> 
> (Updated 2011-04-29 03:15:23)
> 
> 
> Review request for qpid.
> 
> 
> Summary
> -------
> 
> Any execution exception received will now be notified to AMQSession_0_10 via the SessionListener.
> Also the AMQSession_0_10 will be marked closed.
> 
> The Exception thrown when the session is accessed after it was closed, will now contain
information about the underlying AMQ Exception that caused the session closure.
> 
> 
> This addresses bug QPID-3233.
>     https://issues.apache.org/jira/browse/QPID-3233
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
1097636 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1097636 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java
1097636 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/message/TestAMQSession.java
1097636 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java
1097636 
> 
> Diff: https://reviews.apache.org/r/676/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> rajith
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message