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