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