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: Concurrently executing connections are allowed to use the same client ID
Date Thu, 07 Jul 2011 17:05:19 GMT


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

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.


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


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