> On June 25, 2014, 8:48 a.m., Pavel Moravec wrote: > > All the automated test failures were due to a bug in > > cpp/src/tests/brokertest.py that allowed connection option "protocol" to be > > set to amqp1.0 even for native / 0-10 client. Below patch makes all tests > > passing: > > > > Index: cpp/src/tests/brokertest.py > > =================================================================== > > --- cpp/src/tests/brokertest.py (revision 1605024) > > +++ cpp/src/tests/brokertest.py (working copy) > > -344,9 +344,11 @@ > > @param native if True force use of the native qpid.messaging client > > even if swig client is available. > > """ > > - if self.test.protocol: kwargs.setdefault("protocol", > > self.test.protocol) > > if native: connection_class = qpid.messaging.Connection > > - else: connection_class = qm.Connection > > + else: > > + connection_class = qm.Connection > > + if (self.test.protocol and qm == qpid_messaging): > > + kwargs.setdefault("protocol", self.test.protocol) > > return connection_class.establish(self.host_port(), > > timeout=timeout, **kwargs) > > > > @property > > > > Until there will be some feedback, I will commit both the patch above and > > the connection option handling within a day or two.
Good catch, my bad! - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22864/#review46611 ----------------------------------------------------------- On June 24, 2014, 1:01 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22864/ > ----------------------------------------------------------- > > (Updated June 24, 2014, 1:01 p.m.) > > > Review request for qpid, Alan Conway, Kenneth Giusti, Darryl Pierce, and > Rafael Schloming. > > > Bugs: QPID-5836 > https://issues.apache.org/jira/browse/QPID-5836 > > > Repository: qpid > > > Description > ------- > > options need to be iterated to identify possible unknown options. Default > values (even None, otherwise Python complains it does not know them) have to > be set before the loop that overwrites the defaults. > > This is in my eyes better option than adding an extra check like: > > for key in options.keys(): > if key not in ["reconnect", "transport", "port", .. ]: > raise error > > As this check would have to be maintained and there is a risk one forgets to > add a new connection option there while handling it otherwise. > > The code change is more than reasonable but several automated tests failed - > some of them (like ha_tests.RecoveryTests.test_queue_hold) due to connection > option protocol:amqp1.0. While that option should be present _only_ for swig > client that does not call endpoints.py and Connection class. Other test > failures are not directly related to that "missing" option but I guess it > would be the indirect reason (some connection supposed to provision something > failed due to that option, so the provisioning was missing etc.). > > Any idea where & why _native_ client sets protocol:amqp1.0 ? (I even added > the possibility protocol:amqp0-10 but that didnt help, obviously). Automated > tests results added. > > > Diffs > ----- > > /trunk/qpid/python/qpid/messaging/endpoints.py 1604713 > > Diff: https://reviews.apache.org/r/22864/diff/ > > > Testing > ------- > > Manual test with connection option {'a':'b'} raises error: > > Traceback (most recent call last): > File "get_queue_arguments.py", line 17, in <module> > connection = Connection(brokerurl, **parms) > File "/usr/lib/python2.6/site-packages/qpid/messaging/endpoints.py", line > 195, in __init__ > raise ConnectionError("Unknown connection option " + key + " with value " > + value) > qpid.messaging.exceptions.ConnectionError: None(Unknown connection option a > with value b) > > > Thanks, > > Pavel Moravec > >