> On June 23, 2014, 3:31 p.m., Alan Conway wrote: > > /trunk/qpid/python/qpid/messaging/endpoints.py, line 134 > > <https://reviews.apache.org/r/22864/diff/2/?file=614978#file614978line134> > > > > 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.
> On June 23, 2014, 3:31 p.m., Alan Conway wrote: > > /trunk/qpid/python/qpid/messaging/endpoints.py, line 169 > > <https://reviews.apache.org/r/22864/diff/2/?file=614978#file614978line169> > > > > 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... reconnect_interval is outside "self" in current code as well, but due to code clarity it makes sense to add it to "self" scope. - Pavel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22864/#review46401 ----------------------------------------------------------- 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 > >
