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/10/22 15:38:07 UTC
Review Request 27032: QPID-6177: qpid-tool should print warning when
initial connection to broker fails
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/
-----------------------------------------------------------
Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
Bugs: QPID-6177
https://issues.apache.org/jira/browse/QPID-6177
Repository: qpid
Description
-------
Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
Diffs
-----
trunk/qpid/extras/qmf/src/py/qmf/console.py 1633165
trunk/qpid/tools/src/py/qpid-tool 1633165
Diff: https://reviews.apache.org/r/27032/diff/
Testing
-------
Testing with invalid hostname:
$ ./tools/src/py/qpid-tool invalidHost
Management Tool for QPID
qpid: Failed to connect: [Errno -2] Name or service not known
Testing with no SASL mech in common:
$ ./tools/src/py/qpid-tool
Management Tool for QPID
qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
Thanks,
Pavel Moravec
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Pavel Moravec <pm...@redhat.com>.
> On Oct. 22, 2014, 2:39 p.m., Ernie Allen wrote:
> > ```
> > def brokerConnectionFailed(self, broker, e):
> > """ Invoked when a connection to a broker fails """
> > if self.first_connect:
> > self.first_connect = None
> > print "Failed to connect: ", e
> > ```
> > If you set self.first_connect to None here there will be no "Broker connected:" message when the broker does actually connect.
> > You may want a separate variable to control the printing of the "Failed to connect:" message.
I see it opposite: when console fails to connect (and it has not been connected yet, otherwise brokerConnected has been called), the "Failed to connect" is printed out (just once) and first_connect is set to None. If further conn.attempt wont succeed, we dont print it again (intentional). If the attempt will succeed, brokerConnected method will print "Broker connected" message.
I have tried this already. When starting qpid-tool before qpidd:
$ ./tools/src/py/qpid-tool
Management Tool for QPID
qpid: Failed to connect: Exception during connection setup: error - [Errno 111] Connection refused
Broker connected: Broker connected at: localhost:5672
When restarting broker while qpid-tool was connected:
$ ./tools/src/py/qpid-tool
Management Tool for QPID
qpid: Broker disconnected: Disconnected Broker
Broker connected: Broker connected at: localhost:5672
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57787
-----------------------------------------------------------
On Oct. 23, 2014, 7:52 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 7:52 a.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Ernie Allen <ea...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57787
-----------------------------------------------------------
```
def brokerConnectionFailed(self, broker, e):
""" Invoked when a connection to a broker fails """
if self.first_connect:
self.first_connect = None
print "Failed to connect: ", e
```
If you set self.first_connect to None here there will be no "Broker connected:" message when the broker does actually connect.
You may want a separate variable to control the printing of the "Failed to connect:" message.
- Ernie Allen
On Oct. 22, 2014, 1:38 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 1:38 p.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/extras/qmf/src/py/qmf/console.py 1633165
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Pavel Moravec <pm...@redhat.com>.
> On Oct. 22, 2014, 2:59 p.m., Kenneth Giusti wrote:
> > trunk/qpid/extras/qmf/src/py/qmf/console.py, line 57
> > <https://reviews.apache.org/r/27032/diff/1/?file=728747#file728747line57>
> >
> > I think this change breaks the API. The Console object is a public interface and it is expected to be inherited by the user's application. Currently, any application that uses the brokerConnectionFailed will define it with only one argument. If console.py invokes it with two (adding the new 'e' parameter), won't that cause the application to fail?
>
> Kenneth Giusti wrote:
> Just a quick follow up - the error doesn't have to be passed explicitly - there is a .error on the passed broker itself that contains the error string, and .conn_exc contains the exception itself.
Ah thanks. I checked "broker" object there but havent seen the .error or .conn_exc variables there (had to overlook them) - that's why I added the extra param. Now I removed it back.
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57795
-----------------------------------------------------------
On Oct. 23, 2014, 7:52 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 7:52 a.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Kenneth Giusti <kg...@apache.org>.
> On Oct. 22, 2014, 2:59 p.m., Kenneth Giusti wrote:
> > trunk/qpid/extras/qmf/src/py/qmf/console.py, line 57
> > <https://reviews.apache.org/r/27032/diff/1/?file=728747#file728747line57>
> >
> > I think this change breaks the API. The Console object is a public interface and it is expected to be inherited by the user's application. Currently, any application that uses the brokerConnectionFailed will define it with only one argument. If console.py invokes it with two (adding the new 'e' parameter), won't that cause the application to fail?
Just a quick follow up - the error doesn't have to be passed explicitly - there is a .error on the passed broker itself that contains the error string, and .conn_exc contains the exception itself.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57795
-----------------------------------------------------------
On Oct. 22, 2014, 1:38 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 1:38 p.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/extras/qmf/src/py/qmf/console.py 1633165
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57795
-----------------------------------------------------------
trunk/qpid/extras/qmf/src/py/qmf/console.py
<https://reviews.apache.org/r/27032/#comment98670>
I think this change breaks the API. The Console object is a public interface and it is expected to be inherited by the user's application. Currently, any application that uses the brokerConnectionFailed will define it with only one argument. If console.py invokes it with two (adding the new 'e' parameter), won't that cause the application to fail?
- Kenneth Giusti
On Oct. 22, 2014, 1:38 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 1:38 p.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/extras/qmf/src/py/qmf/console.py 1633165
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57981
-----------------------------------------------------------
Ship it!
Ship It!
- Kenneth Giusti
On Oct. 23, 2014, 7:52 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 7:52 a.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Ernie Allen <ea...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/#review57985
-----------------------------------------------------------
Ship it!
Ship It!
- Ernie Allen
On Oct. 23, 2014, 7:52 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27032/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 7:52 a.m.)
>
>
> Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
>
>
> Bugs: QPID-6177
> https://issues.apache.org/jira/browse/QPID-6177
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
>
> Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
>
> As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
>
>
> Diffs
> -----
>
> trunk/qpid/tools/src/py/qpid-tool 1633165
>
> Diff: https://reviews.apache.org/r/27032/diff/
>
>
> Testing
> -------
>
> Testing with invalid hostname:
>
> $ ./tools/src/py/qpid-tool invalidHost
> Management Tool for QPID
> qpid: Failed to connect: [Errno -2] Name or service not known
>
>
> Testing with no SASL mech in common:
>
> $ ./tools/src/py/qpid-tool
> Management Tool for QPID
> qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 27032: QPID-6177: qpid-tool should print warning
when initial connection to broker fails
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27032/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 7:52 a.m.)
Review request for qpid, Ernie Allen, Kenneth Giusti, and Ted Ross.
Changes
-------
Simplified the patch per Ken's comment.
Bugs: QPID-6177
https://issues.apache.org/jira/browse/QPID-6177
Repository: qpid
Description
-------
Simple patch that overwrites default empty brokerConnectionFailed method raised when connection fails. It makes sense to react only when connecting for the first time.
Changes in package imports are irrelevant to this JIRA - I had to do the change to let qpid-tool to start working on trunk (?).
As the logic of QMF console is to retry failed connections, I suggest just raising some warning than directly stopping qpid-tool.
Diffs (updated)
-----
trunk/qpid/tools/src/py/qpid-tool 1633165
Diff: https://reviews.apache.org/r/27032/diff/
Testing
-------
Testing with invalid hostname:
$ ./tools/src/py/qpid-tool invalidHost
Management Tool for QPID
qpid: Failed to connect: [Errno -2] Name or service not known
Testing with no SASL mech in common:
$ ./tools/src/py/qpid-tool
Management Tool for QPID
qpid: Failed to connect: (None, 'sasl negotiation failed: no mechanism agreed')
Thanks,
Pavel Moravec