> On 2011-07-07 16:28:47, Robbie Gemmell wrote: > > This seems a bit horrible, deliberately leaving a session open and unused. > > Were there really no alternative options? > > > > On first glance I also don't imagine this works with the Java broker, which > > would admittedly be the brokers fault but has it been tested? There is a > > MaxChannels value that gets negotiated with the broker during the AMQP > > connection, and the the client currently uses it to provide feedback to > > users when they exceed the allowed number of JMS Sessions, I think this > > will probably break that and allow more Sessions to be created/attempted > > than should be. > > > > When the clientid is being verified the method declares it throws > > JMSException from the delegate, but Exception is caught by the calling > > method instead. Would a boolean return not suffice here, with exceptions > > only being thrown from the verification method due to unexpected occurences? > > Gordon Sim wrote: > Re "This seems a bit horrible, deliberately leaving a session open and > unused. Were there really no alternative options?" - I'm to blame for that > approach! > > I think if possible using a standard feature of the protocol is > desirable. The only alternative I could see was some Qpid specific extension, > e.g. have the client id in the properties on start-ok and have the broker > understand this and reject duplicates (would need to reuse one of the limited > close codes here though which isn't ideal either). > > An AMQP session is generally quite lightweight. Yes, it uses up one > channel, but that's really all. The upside is that it should work for any > compliant 0-10 broker. > > However, I'd genuinely be interested in alternative approaches if you > have any suggestions. > > rajith attapattu wrote: > As for the approach - I am quite keen on using something that works with > any 0-10 complaint broker, rather than implementing anything qpid specific. > This workaround was needed to ensure that the modification was only client > side. The other alternative that Gordon mentioned requires modification to > both brokers and has the disadvantage of not working with non qpid brokers. > > I also didn't quite understand about how this would cause more sessions > to be created than allowed? The session created here is using the same method > as any other session would. So it contains all the necessary checks and > balances, including getting a channelID, registering with the connection, > recreating after failover, closed when the connection is closed ..etc > > As for throwing the JMSException instead of using a boolean. > The createSession method throws a JMSException, so either I need to > handle it or pass it up the stack. I was also hoping that I don't have to > handle the JMSException at all in the verifyClientID method in AMQConnection. > But unfortunately we don't throw a JMSException in the AMQConnection ctor, > rather an AMQException. > So it's the same story - either I handle it at the delegate level or at > the AMQConnection level. Both looks ugly. > > I think a better solution is to have the ctor in AMQConnection throw a > JMSException. Our client should only throw JMSException to the application, > not any Qpid specific exceptions. > > Robbie Gemmell wrote: > Yep I was wrong about the too many Sessions, not paying attention ;-) It > just means that from the users perspective they will just be able to create 1 > fewer Session than if the feature is disabled, so if they know the limit is N > then they will get an exception after their N-1'th Session. > > Regarding the exception, I'm not talking about whether to handle it or > pass it up, just that you are catching all Exceptions when you handle it and > not just the JMSException you are declaring/expecting the verify method > throws. That means you are catching exceptions that may have nothing to do > with the verification indicating the clientid was duplicated, but just > reporting that as the problem regardless. >
This feature is disabled by default :) - so will not catch anybody by surprise. We can add a note in the doc about the max channel thing. The same situation arises with fialover exchange, where it creates a new session to receive updates from the failover exchange. I now understand your comment about the exception handling - I will change the code to specifically catch the JMSException instead of just an exception. If there are no further concerns I will be committing the code tonight. It seems we all agree that the approach taken is no worse than the suggested alternatives and probably less simpler than most. - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review989 ----------------------------------------------------------- On 2011-07-07 02:17:42, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1027/ > ----------------------------------------------------------- > > (Updated 2011-07-07 02:17:42) > > > Review request for qpid, Gordon Sim and Robbie Gemmell. > > > Summary > ------- > > In order to verify the uniqueness of the client ID, a dummy session is > created using client ID as it's name. This prevents any other connection from > using same client ID as the session creation will fail. However this > verification is switched off by default in order to preserve backwards > compatibility. You need to use -Dqpid.verify_client_id=true switch > verification on. > > In summary the following changes were made in order to support the above, > 1. A verifyClientID method was added to the connection delegates, > 2. AMQSession_0_10.java was modified to allow a name to be specified for the > underlying AMQP session. > 3. A method was added to o.a.q.transport.Session.java to wait until the > session state was changed from NEW to OPEN (or another state which triggers > the error). > 4. Setter/Getter in Session.java to store/retrieve the SessionDetachCode and > ConnectionDelegate to set the detach code. > 5. SessionDelegate to notify Session object when attached/dettached/closed is > invoked. > > > This addresses bug QPID-3269. > https://issues.apache.org/jira/browse/QPID-3269 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_8_0.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/XASessionImpl.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionDelegate.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java > 1143628 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java > 1143628 > > Diff: https://reviews.apache.org/r/1027/diff > > > Testing > ------- > > A patch containing a test will be attached to the JIRA shortly. > > > Thanks, > > rajith > >
