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 Fri, 08 Jul 2011 22:32:03 GMT


> On 2011-07-07 12:14:51, Gordon Sim wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java,
line 1069
> > <https://reviews.apache.org/r/1027/diff/1/?file=22094#file22094line1069>
> >
> >     Could you not have used sync() here instead of a new method?
> 
> rajith attapattu wrote:
>     I did wonder about this myself. But once a session is detached you can no longer
sync on it right (from the broker side) ? (Bcos you need a valid session to sync on). 
>     Besides when the session is detached it's marked close on the client side, so sync
wouldn't work anyways as we will throw a session closed exception.
> 
> Gordon Sim wrote:
>     The point would be though that if you are sync()ing on the client side and the session
is detached, that already wakes you up and throws an exception, right?
>     
>     I don't understand the second sentence. However your new method is throwing a session
closed exception as well.
> 
> rajith attapattu wrote:
>     Gordon, my bad, you don't need a new method sync would suffice. 
>     I just tested  using sync with a minor modification and it works.

Upon further testing it seems that sync() is not a good candidate here.
The condition used by sync to go into a wait position is that maxComplete (The commands completed
so far) is less than point (the command you want to sync on).
However when we create the session both maxComplete and point are the same (i.e it's at -1)
and hence it will not wait.

We can add another condition to make sync wait if point = -1.
Also in order to make it work, we need notify sync wait when we receive a detach/attach/close
- so those methods would need to notify on the commands object.
So can make sync work with some modifications I hinted above, but that would need more testing
to ensure it doesn't have any either unintended side effects.

sync() is used in the critical path a lot and at this juncture I think it's best to use new
method rather than tinkering with sync().


- rajith


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


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