> On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, > > line 38 > > <https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38> > > > > I believe you forgot to add ClientConnectionDelegate ? > > I can't seem to find this in the current source tree. > > > > Perhaps some of the missing code from ClientDelegate is moved to this > > class?
Well damn, thatll teach me to say something looks fine without looking at the diff to make sure its actually what I looked it ;( Pretty much all of your comments stem from this one issue as the missing code will indeed be in the ClientConnectionDelegate, which is used in place of ClientDelegate for actual client use now as can be seen further down the class. ClientDelegate is really mainly used in ConnectionTest after this (the same way ServerDelegate was really only used in ConnectionTest, but the real broker uses the ServerConnectionDelegate subclass). The diff posted to ReviewBoard is basically incomplete for some reason. I dont have a copy of the original patch that I actually looked over a couple weeks ago and Keith is on holiday just now, but I'm sure he will post the corrected version on his return next week...until then, no need to test what wont evne compile... :S > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java, > > line 61 > > <https://reviews.apache.org/r/1608/diff/1/?file=34091#file34091line61> > > > > I would think it's better to default to PLAIN as that would be the one > > that will be universally supported. > > However I believe you now retrieve the default from the sasl config > > file ? It doesnt default to a particular mechanism now, and instead chooses from all of the supported mechanisms of the client like the 0-8/9 codebase did (though the changes also improves that to be deterministic). The null default here indicates the user hasnt asked for a specific set of mechs to be the supported ones, whereas a non-null result will be used to override the clients supported list and specify exactly what should be used as the supported mechs, as it did previously. - Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1767 ----------------------------------------------------------- On 2011-08-22 08:58:27, Keith Wall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1608/ > ----------------------------------------------------------- > > (Updated 2011-08-22 08:58:27) > > > Review request for qpid and rajith attapattu. > > > Summary > ------- > > This patch changes the 0-10 code path to create the SASL callback handler > using the CallbackHandlerRegistry. This allows the 0-10 code path to > support SASL mechanisms requiring other callback handlers, such as > CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has > been retained and now applies to the 0-8..0-9-1 code paths too. > > If the user *specifies* a sasl_mechs client connection option the behaviour > of the code is unchanged from the previous version: it restricts the list of > SASL mechanisms in use. > > If the user *does not specify* a sasl_mechs client connection option, the old > code used a hardcoded PLAIN default. This is no longer the case. Now the > client will use the first SASL mechanism from the list > CallbackHandlerRegistry.properties that is also available on the server. > > Removed dead code and strengthen unit tests. > > > > This addresses bug QPID-3415. > https://issues.apache.org/jira/browse/QPID-3415 > > > Diffs > ----- > > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java > 1160136 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java > 1160136 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java > 1160136 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties > 1160136 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java > 1160136 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java > 1160136 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java > 1160136 > > /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java > 1160136 > > /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java > 1160136 > > /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java > 1160136 > > Diff: https://reviews.apache.org/r/1608/diff > > > Testing > ------- > > Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to > test GSSAPI locally. > > > Thanks, > > Keith > >
