qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gordon Sim" <g...@redhat.com>
Subject Re: Review Request: Add log entries for correlatable broker object life cycles
Date Tue, 03 Jul 2012 11:04:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5616/#review8831
-----------------------------------------------------------


I feel this could be cleaner if contained within management code. There are already events
raised for creation/deletion of queues and exchanges (creation admittedly is muddled by the
event being a declare event, but I would rather fix that anyway), subscribe/unsubscribe, bind/unbind.
If for some reason those events can't be used to also issue log statements, perhaps the management
object creation acan be used in a generic way as for deletion here.

I'm not sure I get the philosophy of when connection ids are desired. Queue and Exchange creation
seems to want that; deletion does not(?). Bindings don't contain any information. Subscriptions
log the session id. The connection ids are lost on a restart anyway. However if they must
be logged against queue and exchange creation I think that can be done more simply (i.e. less
changes). (One option is using the existing qmf events; another is suggested in comments on
exchange registry below, i.e. do it there since all that is logged is name and connection
id).


trunk/qpid/cpp/include/qpid/SessionId.h
<https://reviews.apache.org/r/5616/#comment18743>

    I think adding this here is wrong. A session id does not logically include the connection
id. See suggestion in SessionState changes for simple alternative.



trunk/qpid/cpp/src/qpid/acl/Acl.cpp
<https://reviews.apache.org/r/5616/#comment18732>

    Use getStatsAsMap()? (For consistency more than real performance concerns)



trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/5616/#comment18733>

    Use getStatsAsMap()?



trunk/qpid/cpp/src/qpid/broker/Exchange.cpp
<https://reviews.apache.org/r/5616/#comment18734>

    Why does the name need to be passed in here? The management object was given the name
when it was created.



trunk/qpid/cpp/src/qpid/broker/Exchange.cpp
<https://reviews.apache.org/r/5616/#comment18735>

    Assuming the '[Protocol]' string indicates the exchange was declared via an AMQP 0-10
command:
    
    (a) how useful is that anyway? is this as opposed to creating it via the Broker::create()
QMF command? What is the significance of that dinstinction? (It will no longer apply for AMQP
1.0).
    
    (b) in this case it is actually potentially inaccurate. If the durable record does not
include the manner in which it was created, then the distinction is lost on recovery anyway.
(Which brings us back to (a) :-)



trunk/qpid/cpp/src/qpid/broker/Exchange.cpp
<https://reviews.apache.org/r/5616/#comment18736>

    I notice we are not recording connection id here...



trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp
<https://reviews.apache.org/r/5616/#comment18737>

    Since the purpose of passing through the conection id to each exchange types constructor
is simply to have a log statement with the exchange name and connection id, why no simply
log that here and avoid having to pass it into the exchanges themselves?



trunk/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/5616/#comment18738>

    I am of the opinion that the '[restore]' renders this added field rather meaningless in
the general case (and can't see that it is of great significance anyway).



trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/5616/#comment18739>

    Again, why does the name need to be passed in here?



trunk/qpid/cpp/src/qpid/broker/SessionState.cpp
<https://reviews.apache.org/r/5616/#comment18740>

    Why add the connection id to the session id rather than just retrieving the connection
id at this point via SessionState::getConnection() and ConnectionState::getUrl()?



trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp
<https://reviews.apache.org/r/5616/#comment18741>

    I don't understand this change; could you clarify?



trunk/qpid/cpp/src/qpid/management/ManagementObject.cpp
<https://reviews.apache.org/r/5616/#comment18742>

    Why not have the Created log entry done in the management object as well? And why not
just always use the management key?


- Gordon Sim


On July 2, 2012, 9:11 p.m., Chug Rolke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5616/
> -----------------------------------------------------------
> 
> (Updated July 2, 2012, 9:11 p.m.)
> 
> 
> Review request for qpid, Alan Conway, Gordon Sim, and Ted Ross.
> 
> 
> Description
> -------
> 
> This patch adds a new log category [Configuration] and publishes a bunch of information
at info level in the format:
> [Configuration] <object> <event>, where <object> is one of Connection,
Session, Queue, Subscription, Exchange or Binding, and <event> is one of created or
closed with connection also getting a setUser.
> The bulk of the patch involves passing the necessary strings down to the object creators
so that they can emit the log.
> 
> 
> This addresses bug QPID-4079.
>     https://issues.apache.org/jira/browse/QPID-4079
> 
> 
> Diffs
> -----
> 
>   trunk/qpid/cpp/include/qpid/SessionId.h 1354515 
>   trunk/qpid/cpp/include/qpid/log/Statement.h 1354515 
>   trunk/qpid/cpp/include/qpid/management/ManagementObject.h 1354515 
>   trunk/qpid/cpp/managementgen/qmfgen/templates/Class.h 1354515 
>   trunk/qpid/cpp/managementgen/qmfgen/templates/Class.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/SessionId.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/acl/Acl.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Broker.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/DirectExchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/DirectExchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Exchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Exchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/FanOutExchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/FanOutExchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/HeadersExchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/HeadersExchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Link.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Queue.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/QueueRegistry.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/SessionHandler.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/broker/TopicExchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/broker/TopicExchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/log/Statement.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/management/ManagementObject.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/replication/ReplicatingEventListener.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/xml/XmlExchange.h 1354515 
>   trunk/qpid/cpp/src/qpid/xml/XmlExchange.cpp 1354515 
>   trunk/qpid/cpp/src/qpid/xml/XmlExchangePlugin.cpp 1354515 
>   trunk/qpid/cpp/src/tests/ExchangeTest.cpp 1354515 
> 
> Diff: https://reviews.apache.org/r/5616/diff/
> 
> 
> Testing
> -------
> 
> Passes cmake test and automake make check.
> 
> 
> Thanks,
> 
> Chug Rolke
> 
>


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