You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2014/06/23 16:35:34 UTC
Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22864/
-----------------------------------------------------------
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
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Alan Conway <ac...@redhat.com>.
> On June 25, 2014, 8:48 a.m., Pavel Moravec wrote:
> > All the automated test failures were due to a bug in cpp/src/tests/brokertest.py that allowed connection option "protocol" to be set to amqp1.0 even for native / 0-10 client. Below patch makes all tests passing:
> >
> > Index: cpp/src/tests/brokertest.py
> > ===================================================================
> > --- cpp/src/tests/brokertest.py (revision 1605024)
> > +++ cpp/src/tests/brokertest.py (working copy)
> > -344,9 +344,11 @@
> > @param native if True force use of the native qpid.messaging client
> > even if swig client is available.
> > """
> > - if self.test.protocol: kwargs.setdefault("protocol", self.test.protocol)
> > if native: connection_class = qpid.messaging.Connection
> > - else: connection_class = qm.Connection
> > + else:
> > + connection_class = qm.Connection
> > + if (self.test.protocol and qm == qpid_messaging):
> > + kwargs.setdefault("protocol", self.test.protocol)
> > return connection_class.establish(self.host_port(), timeout=timeout, **kwargs)
> >
> > @property
> >
> > Until there will be some feedback, I will commit both the patch above and the connection option handling within a day or two.
Good catch, my bad!
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22864/#review46611
-----------------------------------------------------------
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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22864/#review46611
-----------------------------------------------------------
All the automated test failures were due to a bug in cpp/src/tests/brokertest.py that allowed connection option "protocol" to be set to amqp1.0 even for native / 0-10 client. Below patch makes all tests passing:
Index: cpp/src/tests/brokertest.py
===================================================================
--- cpp/src/tests/brokertest.py (revision 1605024)
+++ cpp/src/tests/brokertest.py (working copy)
-344,9 +344,11 @@
@param native if True force use of the native qpid.messaging client
even if swig client is available.
"""
- if self.test.protocol: kwargs.setdefault("protocol", self.test.protocol)
if native: connection_class = qpid.messaging.Connection
- else: connection_class = qm.Connection
+ else:
+ connection_class = qm.Connection
+ if (self.test.protocol and qm == qpid_messaging):
+ kwargs.setdefault("protocol", self.test.protocol)
return connection_class.establish(self.host_port(), timeout=timeout, **kwargs)
@property
Until there will be some feedback, I will commit both the patch above and the connection option handling within a day or two.
- Pavel Moravec
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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
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.
Changes
-------
New patch version with incorporated Alan's comments.
Automated tests failures (will have a look on them):
1)
ha_tests.RecoveryTests.test_queue_hold .................................. fail
Error during test: Traceback (most recent call last):
File "/data_xfs/qpid/cpp/BLD/src/tests/python/commands/qpid-python-test", line 340, in run
phase()
File "/data_xfs/qpid/cpp/src/tests/ha_tests.py", line 1136, in test_queue_hold
s1 = cluster.connect(0, native=True).session().sender("q1;{create:always}")
File "/data_xfs/qpid/cpp/src/tests/ha_test.py", line 353, in connect
return self[i].connect(reconnect=True, reconnect_urls=self.url.split(","), **kwargs)
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 350, in connect
return connection_class.establish(self.host_port(), timeout=timeout, **kwargs)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 67, in establish
conn = Connection(url, **options)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 161, in __init__
raise ConnectionError("Connection option 'protocol' value '" + value + "' unsupported (must be amqp0-10)")
TypeError: cannot concatenate 'str' and 'bool' objects
2) _all_ qpidd_qmfv2_tests failed like this one:
qpidd_qmfv2_tests.ConsoleTest.test_async_method ......................... fail
Error during test: Traceback (most recent call last):
File "/data_xfs/qpid/cpp/BLD/src/tests/python/commands/qpid-python-test", line 340, in run
phase()
File "/data_xfs/qpid/cpp/src/tests/qpidd_qmfv2_tests.py", line 254, in test_async_method
self._startBroker()
File "/data_xfs/qpid/cpp/src/tests/qpidd_qmfv2_tests.py", line 53, in _startBroker
self.broker = BrokerTest.broker(self, args)
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 531, in broker
raise RethrownException("Failed to start broker %s(%s): %s" % (b.name, b.log, e))
RethrownException: Failed to start broker broker0(000:broker0.log): Broker broker0 not responding: (None(Connection option 'protocol' value 'amqp1.0' unsupported (must be amqp0-10))):
2014-06-24 13:18:11 [Store] notice Journal "TplStore": Created
2014-06-24 13:18:11 [Store] notice Store module initialized; store-dir=broker0
2014-06-24 13:18:11 [Broker] notice SASL disabled: No Authentication Performed
2014-06-24 13:18:11 [Network] notice Listening on TCP/TCP6 port 37940
2014-06-24 13:18:11 [Broker] notice Broker running
Traceback (most recent call last):
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 408, in ready
c = self.connect(timeout=timeout, **kwargs)
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 350, in connect
return connection_class.establish(self.host_port(), timeout=timeout, **kwargs)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 67, in establish
conn = Connection(url, **options)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 161, in __init__
raise ConnectionError("Connection option 'protocol' value '" + value + "' unsupported (must be amqp0-10)")
ConnectionError: None(Connection option 'protocol' value 'amqp1.0' unsupported (must be amqp0-10))
Traceback (most recent call last):
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 529, in broker
try: b.ready()
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 417, in ready
self.name,e,error_line(self.log, 5)))
RethrownException: Broker broker0 not responding: (None(Connection option 'protocol' value 'amqp1.0' unsupported (must be amqp0-10))):
2014-06-24 13:18:11 [Store] notice Journal "TplStore": Created
2014-06-24 13:18:11 [Store] notice Store module initialized; store-dir=broker0
2014-06-24 13:18:11 [Broker] notice SASL disabled: No Authentication Performed
2014-06-24 13:18:11 [Network] notice Listening on TCP/TCP6 port 37940
2014-06-24 13:18:11 [Broker] notice Broker running
Traceback (most recent call last):
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 408, in ready
c = self.connect(timeout=timeout, **kwargs)
File "/data_xfs/qpid/cpp/src/tests/brokertest.py", line 350, in connect
return connection_class.establish(self.host_port(), timeout=timeout, **kwargs)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 67, in establish
conn = Connection(url, **options)
File "/data_xfs/qpid/cpp/BLD/src/tests/python/qpid/messaging/endpoints.py", line 161, in __init__
raise ConnectionError("Connection option 'protocol' value '" + value + "' unsupported (must be amqp0-10)")
ConnectionError: None(Connection option 'protocol' value 'amqp1.0' unsupported (must be amqp0-10))
3) Same error in all legacystore_python_tests tests
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 (updated)
-----
/trunk/qpid/python/qpid/messaging/endpoints.py 1604713
Diff: https://reviews.apache.org/r/22864/diff/
Testing (updated)
-------
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
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Alan Conway <ac...@redhat.com>.
> 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...
>
> Pavel Moravec wrote:
> reconnect_interval is outside "self" in current code as well, but due to code clarity it makes sense to add it to "self" scope.
Actually there's no need, I should have ready more carefully. reconnect_interval is only used locally as a default for reconnect_interval_min/max -those are the variables that are used later in driver.py to set up reconnect delays. So there's no need for it to be stored on self.
- Alan
-----------------------------------------------------------
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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Alan Conway <ac...@redhat.com>.
> 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...
>
> Pavel Moravec wrote:
> reconnect_interval is outside "self" in current code as well, but due to code clarity it makes sense to add it to "self" scope.
>
> Alan Conway wrote:
> Actually there's no need, I should have ready more carefully. reconnect_interval is only used locally as a default for reconnect_interval_min/max -those are the variables that are used later in driver.py to set up reconnect delays. So there's no need for it to be stored on self.
Sorry, I take that back. In the new scheme of things it is being set on self as are all the options - it's clearer and simpler to leave it there.
- Alan
-----------------------------------------------------------
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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Pavel Moravec <pm...@redhat.com>.
> 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
>
>
Re: Review Request 22864: [Python client] Report error when encountering
unrecognized connection option
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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
>
>