----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1027/#review985 -----------------------------------------------------------
Couple of minor points below. In general looks ok to me though I am not as familiar with the codebase as a whole as I should be. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java <https://reviews.apache.org/r/1027/#comment2051> What if any is the implication for failover? The session object is scoped to this method. Is that intended? There is a reference to it held somewhere else? Will it be cleaned up correctly? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java <https://reviews.apache.org/r/1027/#comment2050> Could you not have used sync() here instead of a new method? - Gordon 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 > >
