qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gordon Sim <g...@redhat.com>
Subject Re: svn commit: r1052086 - in /qpid/trunk/qpid: extras/qmf/src/py/qmf/console.py tools/src/py/qpid-config tools/src/py/qpid-printevents tools/src/py/qpid-stat tools/src/py/qpid-tool
Date Wed, 05 Jan 2011 15:05:31 GMT
On 01/04/2011 10:26 PM, Jonathan Robie wrote:
> On 01/04/2011 03:47 PM, Gordon Sim wrote:
>
>>> URL: http://svn.apache.org/viewvc?rev=1052086&view=rev
>>> Log:
>>> Added logging to QMF console connections.
>>>
>>> Warning if a broker can not be found, error if SASL authentication
>>> fails or other connection errors when connecting to an existing
>>> broker. Default log level is ERROR.
>>
>> What is the usual default for the log level (i.e. what is the default
>> for other uses of the log module)? It seems a little odd to set the log
>> level to error on most tools, perhaps that's just me...
>
> It's easy to change the default.

 From the commit it looks like each tool sets the 'default' separately(?)

> I'm not sure what the default log level should be for QMF console
> applications - what would you suggest?

What is the usual default (i.e. for other users of the qpid.log module)? 
What other log statements are there for 'qpid.qmf' (I couldn't find any)?

>>> qpid-printevents allows the log level to be set.
>>
>> What about the other tools? Is it not useful for them to also be able to
>> set the logging level? (As the topic of consistency came up recently,
>> should the -v/--verbose option in qpid-route by rationalised with this?)
>
> It would be easy to add the log level option to the other tools.
>
> When I raised the topic of consistency in options recently, you
> suggested we should start by determining which tools we need and
> designing the right tool set. I'd like to avoid making changes that are
> not backwards compatible until we actually do that redesign.
>
> Ideally, I'd probably just use logging levels in qpid-route. But for
> now, I'm reluctant to eliminate -v if there are scripts that might use it.

That is certainly an important consideration and I agree that we would 
not want to start eliminating options without due consideration. I think 
similar consideration before adding any options - particularly any that 
may increase the feeling of inconsistency - is also important.

I guess the real issue is that I don't fully understand the particular 
choices made in this commit or what motivated them.

What was your thinking around a lower log level (i.e. warn rather than 
error) for the case where 'a broker can not be found'? And what exactly 
do you mean by that? I seem to only get the warning level used where the 
hostname can't even be resolved; connection refused and no-route to host 
are both error level statements. This seems pretty odd to me.

The changes to qpid-printevents seem to be addressing a different issue, 
in fact two further separate issues.

The first is a new option that as I understand it disables the built-in 
connection retry (and in an ideal world would have been made via a 
distinct commit). While not opposed to the change in principle, it would 
be good to be clear on the motivation.

The second is an option to alter the logging level for the qpid.qmf 
logger. What's the motivation for this? Are there any log statements for 
that logger other than the one added by this commit? The tool already 
has some sort of logging style output that this option doesn't affect.

I suspect this commit was motivated by the fact that qpid-printevents 
appears just to hang if it can't establish the connection to begin with 
(if it is disconnected, that is already logged, as is subsequent 
reconnection).

>>> It also allows the user to specify that a connection is required, in
>>> which case it terminates if a connection can not be established.
>>
>> I'm not mad on the name of this new option (i.e. require-connection). It
>> seems like allow-reconnect or exit-on-failure would communicate a little
>> more on the purpose of the option.
>
> It's more like exit-on-connection-failure, but that's rather verbose.
>
> To me, exit-on-failure fails to communicate that this has to do with
> connections,

Alternatives in that case might be 
exit-on-disconnect/exit-if-not-connected/exit-unless-connected... 
however it actually also affects handling of other types of error, e.g. 
acl failures, so I'm not sure that exit-on-failure is even wrong.

> and allow-reconnect is not the same as exiting if a
> connection can not be established.

The allow-reconnect option would indicate that it is ok to try to 
periodically reconnect; if not then the only choice would be to exit. 
Reversing the sense, disable-reconnect would also be reasonable in my 
view, and in fact I think that would probably be my preference.

> I'm not convinced that either of
> these really does a better job of explaining the purpose.

I'm fairly convinced that I don't like require-connection.

The tool *always* requires a connection else it cannot do anything. The 
question is whether it is allowed to remain running in the disconnected 
state while periodically trying to re-establish a connection or whether 
it should immediately exit.

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message