----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22864/#review46623 -----------------------------------------------------------
Ship it! Two comments, otherwise it is good to go. /trunk/qpid/python/qpid/messaging/endpoints.py <https://reviews.apache.org/r/22864/#comment82130> This should be: raise ConnectionError("Unknown connection option %s with value %s"%(key, value)) If you use + then you will get a TypeError if the value is not a string. The % conversion will coerce the values to strings str() function. You can see this in the error output in one of your comments: there is a TypeError where it should be ConnectionError. /trunk/qpid/python/qpid/messaging/endpoints.py <https://reviews.apache.org/r/22864/#comment82131> This use of if/else will bomb on python 2.4 which we are still stuck with on RHEL5. It really irritates me not to use it, it is so much neater. - Alan Conway 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 > >
