qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 22864: [Python client] Report error when encountering unrecognized connection option
Date Mon, 23 Jun 2014 15:31:16 GMT

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


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