> 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 > >
