----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22864/#review46401 -----------------------------------------------------------
I have a suggestion to simplify the code, see comments on diff. If ha tests are passing amqp1.0 to the native client that is a bug in the tests. It was non trivial to get the HA tests working with both swig and native clients I'm not surprised if I overlooked something like that. Can't say for sure but if other tests are doing this it is probably a test bug also. /trunk/qpid/python/qpid/messaging/endpoints.py <https://reviews.apache.org/r/22864/#comment81762> You are no longer setting all the attribute to None at the outset. I think that means attribute not mentioned in options won't get set which will cause problems later as the attributes won't exist. /trunk/qpid/python/qpid/messaging/endpoints.py <https://reviews.apache.org/r/22864/#comment81761> Missing self. on reconnect_interval This if statement is long. May I suggest something like: # We specify the full list of attributes in one place. opt_attrs = ['host', 'transport', 'port'...] # Create all attributes on self and set to None. for key in opt_keys: setattr(self, attr, None) # Get values from options, check for invalid options for (key, value) in options.iteritems(): if key in opt_keys: setattr(self, key, value) else: raise ConnectionError("Unknown connection option " + key + " with value " + value) # Now handle items that need special treatment or have speical defaults: if self.host: url = default(url, self.host) if isinstance(url, basestring): url = URL(url) self.host = url.host if self.protocol and self.protocol != "amqp0-10": raise ConnectionError("Connection option 'protocol' value '" + value + "' unsupported (must be amqp0-10)") # ... etc, set defaults... - Alan Conway On June 23, 2014, 2:35 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22864/ > ----------------------------------------------------------- > > (Updated June 23, 2014, 2:35 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 > ------- > > Automated tests passed. > > 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) > > > File Attachments > ---------------- > > automated tests results > > https://reviews.apache.org/media/uploaded/files/2014/06/23/4458e6a3-7d13-445e-a280-4548a6cc21fc__LastTest.log > > > Thanks, > > Pavel Moravec > >
