qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pavel Moravec" <pmora...@redhat.com>
Subject Re: Review Request 22864: [Python client] Report error when encountering unrecognized connection option
Date Wed, 25 Jun 2014 08:43:24 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message