You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2013/12/17 09:04:19 UTC
Review Request 16316: Implement control of internal logging in
qpid::messaging API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/
-----------------------------------------------------------
Review request for qpid, Chug Rolke and Gordon Sim.
Bugs: QPID-5415
https://issues.apache.org/jira/browse/QPID-5415
Repository: qpid
Description
-------
Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
Diffs
-----
/trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
/trunk/qpid/cpp/src/CMakeLists.txt 1551330
/trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551330
/trunk/qpid/cpp/src/qpid/log/Options.h 1551330
/trunk/qpid/cpp/src/qpid/log/Options.cpp 1551330
/trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551330
/trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551330
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
/trunk/qpid/cpp/src/tests/CMakeLists.txt 1551330
/trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16316/diff/
Testing
-------
Added in unit tests which pass*
*They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
Thanks,
Andrew Stitcher
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30539
-----------------------------------------------------------
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58502>
Oops, Good point.
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58503>
Hmm, this is an interesting question.
I don't think this is any worse than registering a callback function and probably slightly more flexible under future change.
You can't change the vtable order or the signature of the callback function in order to allow old code to receive callbacks from a newer library.
But you could add a whole new registration call which causes a new virtual function to be called. As long as the new callback is defined after the previous one in the class, which will cause it to be later in the vtable (at least for gcc although this is not guaranteed).
- Andrew Stitcher
On Dec. 17, 2013, 8:04 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 8:04 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551330
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30527
-----------------------------------------------------------
Ship it!
Couple of questions, just to be sure, but looks good to me.
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58472>
virtual destructor?
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58473>
Are there any ABI issues when relying on a virtual method in the API?
- Gordon Sim
On Dec. 17, 2013, 8:04 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 8:04 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551330
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Alan Conway <ac...@redhat.com>.
> On Dec. 17, 2013, 5:22 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp, line 68
> > <https://reviews.apache.org/r/16316/diff/1/?file=398793#file398793line68>
> >
> > That's really, really weird. I had no idea you could omit the braces in a function definition in any circumstances. Can you do it with anything other than try/catch? I tried a few things and none worked.
> >
> > I think the extra braces might prevent a lot of head-scratching even if they are ugly.
>
> Andrew Stitcher wrote:
> Yes, it only works with a try block.
>
> As I said above it's not really necessary here (but I like it dammit, and it's been in the language a long time now).
>
> It should really be better known for the following (real) use:
>
> The real use case is catching exceptions from initialisers in a constructor viz.
>
> Class::Class()
> try :
> object_with constructor_that_might_throw(42)
> {}
> catch (std::exception e) {
> throw MyClassException(e.what());
> }
>
> That looks even more weird, but get used to it, because it is the only way to catch exceptions in constructor initialisers (and in fact you need something like this to catch exceptions in implicitly called constructors even with no explicit initialiser list)
>
Curiouser and curiouser. I love how C++ is able to solve any problem simply by making the language more complicated :)
Leave it in, it'll be educational.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30543
-----------------------------------------------------------
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Dec. 17, 2013, 5:22 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp, line 68
> > <https://reviews.apache.org/r/16316/diff/1/?file=398793#file398793line68>
> >
> > That's really, really weird. I had no idea you could omit the braces in a function definition in any circumstances. Can you do it with anything other than try/catch? I tried a few things and none worked.
> >
> > I think the extra braces might prevent a lot of head-scratching even if they are ugly.
Yes, it only works with a try block.
As I said above it's not really necessary here (but I like it dammit, and it's been in the language a long time now).
It should really be better known for the following (real) use:
The real use case is catching exceptions from initialisers in a constructor viz.
Class::Class()
try :
object_with constructor_that_might_throw(42)
{}
catch (std::exception e) {
throw MyClassException(e.what());
}
That looks even more weird, but get used to it, because it is the only way to catch exceptions in constructor initialisers (and in fact you need something like this to catch exceptions in implicitly called constructors even with no explicit initialiser list)
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30543
-----------------------------------------------------------
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30543
-----------------------------------------------------------
Ship it!
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp
<https://reviews.apache.org/r/16316/#comment58507>
That's really, really weird. I had no idea you could omit the braces in a function definition in any circumstances. Can you do it with anything other than try/catch? I tried a few things and none worked.
I think the extra braces might prevent a lot of head-scratching even if they are ugly.
- Alan Conway
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Dec. 17, 2013, 5:10 p.m., Alan Conway wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 85
> > <https://reviews.apache.org/r/16316/diff/1/?file=398786#file398786line85>
> >
> > Chuck did some work to provide a set of sensible "subsystem" labels that the pattern can match that might be clearer for a user than function names.
My reply seems to have made it into its own comment.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30540
-----------------------------------------------------------
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30540
-----------------------------------------------------------
Ship it!
Nice job!
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58504>
Chuck did some work to provide a set of sensible "subsystem" labels that the pattern can match that might be clearer for a user than function names.
- Alan Conway
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Jan. 3, 2014, 10:54 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 54
> > <https://reviews.apache.org/r/16316/diff/3/?file=414058#file414058line54>
> >
> > Could do with a comment explaining what 'user' means here.
> >
> > (And though it is not related to the most recent change, the meaning of 'file' in this context would also be worth clarifying).
Agreed - will fix the comment block to explain the parameters
> On Jan. 3, 2014, 10:54 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 143
> > <https://reviews.apache.org/r/16316/diff/3/?file=414058#file414058line143>
> >
> > Comment should make clear that this will be logged in the 'user' category.
The categories aren't visible per se to the application (except that it would be difficult to turn off filtering by category). I'll add a note to say that anything logged this way will set the user flag.
> On Jan. 3, 2014, 10:54 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/log/Statement.h, line 79
> > <https://reviews.apache.org/r/16316/diff/3/?file=414064#file414064line79>
> >
> > I'm not crazy about 'user' as the category name. Not a huge issue, but to me 'application' or 'external' or even 'external_application' would be a bit more obvious. A comment would help clarify regardless.
Agreed - I'll go for external_application for clarity (I thought I add a comment in the block above, I will make sure it actually gets added)
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31121
-----------------------------------------------------------
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Gordon Sim <gs...@redhat.com>.
> On Jan. 3, 2014, 10:54 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 143
> > <https://reviews.apache.org/r/16316/diff/3/?file=414058#file414058line143>
> >
> > Comment should make clear that this will be logged in the 'user' category.
>
> Andrew Stitcher wrote:
> The categories aren't visible per se to the application (except that it would be difficult to turn off filtering by category). I'll add a note to say that anything logged this way will set the user flag.
It was the filtering by category I was thinking about. I think of the 'user' flag as merely indicating to any registered callback that the statement was in that category, is that right(?).
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31121
-----------------------------------------------------------
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Jan. 3, 2014, 10:54 a.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 143
> > <https://reviews.apache.org/r/16316/diff/3/?file=414058#file414058line143>
> >
> > Comment should make clear that this will be logged in the 'user' category.
>
> Andrew Stitcher wrote:
> The categories aren't visible per se to the application (except that it would be difficult to turn off filtering by category). I'll add a note to say that anything logged this way will set the user flag.
>
> Gordon Sim wrote:
> It was the filtering by category I was thinking about. I think of the 'user' flag as merely indicating to any registered callback that the statement was in that category, is that right(?).
That is how it works "under the hood".
I was thinking about it differently though: Over the break I realised that the application might want to treat its own log messages differently from ones coming from the qpid library and it had no way to distinguish programmatically. So adding the extra category was the simplest way to implement that and doesn't add anything really out of the way to the logger code.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31121
-----------------------------------------------------------
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31121
-----------------------------------------------------------
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment59467>
Could do with a comment explaining what 'user' means here.
(And though it is not related to the most recent change, the meaning of 'file' in this context would also be worth clarifying).
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment59470>
Comment should make clear that this will be logged in the 'user' category.
/trunk/qpid/cpp/src/qpid/log/Statement.h
<https://reviews.apache.org/r/16316/#comment59468>
I'm not crazy about 'user' as the category name. Not a huge issue, but to me 'application' or 'external' or even 'external_application' would be a bit more obvious. A comment would help clarify regardless.
/trunk/qpid/cpp/src/qpid/log/Statement.cpp
<https://reviews.apache.org/r/16316/#comment59469>
As above re category name.
- Gordon Sim
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Jan. 3, 2014, 5:38 p.m., Gordon Sim wrote:
> > I do think it would be worth a line explaining that the statements logged by the application itself through this interface will be in the 'Application' category. If anyone actually uses that mechanism you could imagine them wanting more verbosity for their own statements than for those coming from the library (e.g. --log-enable info+:Application --log-enable notice+). They can of course discover this fairly easily themselves.
I've amended the usage message to mention the Application category specifically.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31137
-----------------------------------------------------------
On Jan. 3, 2014, 5:24 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:24 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31137
-----------------------------------------------------------
Ship it!
I do think it would be worth a line explaining that the statements logged by the application itself through this interface will be in the 'Application' category. If anyone actually uses that mechanism you could imagine them wanting more verbosity for their own statements than for those coming from the library (e.g. --log-enable info+:Application --log-enable notice+). They can of course discover this fairly easily themselves.
- Gordon Sim
On Jan. 3, 2014, 5:24 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:24 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/
-----------------------------------------------------------
(Updated Jan. 3, 2014, 5:24 p.m.)
Review request for qpid, Chug Rolke and Gordon Sim.
Changes
-------
Naming changes and extra comments from review feedback
Bugs: QPID-5415
https://issues.apache.org/jira/browse/QPID-5415
Repository: qpid
Description
-------
Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
/trunk/qpid/cpp/src/CMakeLists.txt 1551620
/trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Options.h 1551620
/trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
/trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
/trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
/trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16316/diff/
Testing
-------
Added in unit tests which pass*
*They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
Thanks,
Andrew Stitcher
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31129
-----------------------------------------------------------
Ship it!
Ship It!
- Alan Conway
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review31132
-----------------------------------------------------------
Ship it!
Various bindings need similar additions in order for the logging control to be exposed to the ultimate end user.
- Chug Rolke
On Jan. 3, 2014, 5:48 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2014, 5:48 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/
-----------------------------------------------------------
(Updated Jan. 3, 2014, 5:48 a.m.)
Review request for qpid, Chug Rolke and Gordon Sim.
Changes
-------
I've made a small change to the API to allow the use log callback function to figure out whether the log message it received originally came from the library or from the user code.
I think this is very much in the same direction as the rest of the API and a small extra, so I'll assume the previous "ship it"s apply unless someone shouts soon.
Bugs: QPID-5415
https://issues.apache.org/jira/browse/QPID-5415
Repository: qpid
Description
-------
Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
/trunk/qpid/cpp/src/CMakeLists.txt 1551620
/trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Options.h 1551620
/trunk/qpid/cpp/src/qpid/log/Options.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551620
/trunk/qpid/cpp/src/qpid/log/Statement.h 1551620
/trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551620
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
/trunk/qpid/cpp/src/tests/CMakeLists.txt 1551620
/trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16316/diff/
Testing
-------
Added in unit tests which pass*
*They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
Thanks,
Andrew Stitcher
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Alan Conway <ac...@redhat.com>.
> On Dec. 17, 2013, 5:17 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, line 85
> > <https://reviews.apache.org/r/16316/diff/1/?file=398786#file398786line85>
> >
> > Those are the category names (I didn't want to be much more specific as they aren't really very visible to a user of the qpid messaging library)
> >
> > Indeed I decided not to make the categories visible to this logger at all except that it was hard to avoid allowing filtering on them. I'm willing to reconsider this.
OK, I'd leave it as is.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30541
-----------------------------------------------------------
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30541
-----------------------------------------------------------
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58505>
Those are the category names (I didn't want to be much more specific as they aren't really very visible to a user of the qpid messaging library)
Indeed I decided not to make the categories visible to this logger at all except that it was hard to avoid allowing filtering on them. I'm willing to reconsider this.
- Andrew Stitcher
On Dec. 17, 2013, 5:02 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 5:02 p.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551560
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/
-----------------------------------------------------------
(Updated Dec. 17, 2013, 5:02 p.m.)
Review request for qpid, Chug Rolke and Gordon Sim.
Changes
-------
Updated to address review comments
Bugs: QPID-5415
https://issues.apache.org/jira/browse/QPID-5415
Repository: qpid
Description
-------
Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
Diffs (updated)
-----
/trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
/trunk/qpid/cpp/src/CMakeLists.txt 1551560
/trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551560
/trunk/qpid/cpp/src/qpid/log/Options.h 1551560
/trunk/qpid/cpp/src/qpid/log/Options.cpp 1551560
/trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551560
/trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551560
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
/trunk/qpid/cpp/src/tests/CMakeLists.txt 1551560
/trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/16316/diff/
Testing
-------
Added in unit tests which pass*
*They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
Thanks,
Andrew Stitcher
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Andrew Stitcher <as...@apache.org>.
> On Dec. 17, 2013, 2:46 p.m., Chug Rolke wrote:
> > /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp, line 68
> > <https://reviews.apache.org/r/16316/diff/1/?file=398793#file398793line68>
> >
> > How is this function's body not surrounded with {}? Seems unnatural.
This is just a rarely used part of the language.
True, it's not actually necessary here, but it seems less ugly to me than having the extra '{','}'s.
[The place it *is* necessary is where you want to catch exceptions thrown in the initialisers ofa constructor]
> On Dec. 17, 2013, 2:46 p.m., Chug Rolke wrote:
> > /trunk/qpid/cpp/src/qpid/log/Selector.cpp, line 223
> > <https://reviews.apache.org/r/16316/diff/1/?file=398791#file398791line223>
> >
> > Why can't a user disable critical level?
This is the fix for QPID-5427 (https://issues.apache.org/jira/browse/QPID-5427):
There are 2 rationales for not allowing critical messages to be masked:
Logical: The only purpose of having a log level more important than warning is to tell the user/administrator something that shoudl not be ignored so we should not allow it to be ignored.
Pragmatic: In many (most?) cases where there is a critical error the program will have to stop (or be restarted) stopping with no visible log message (when there could be one) would be a very bad behaviour.
Having said that I'm open to debate on this subject - I suggest to move it to the bug thread though.
> On Dec. 17, 2013, 2:46 p.m., Chug Rolke wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, lines 87-88
> > <https://reviews.apache.org/r/16316/diff/1/?file=398786#file398786line87>
> >
> > make one of these stderr
Oops
> On Dec. 17, 2013, 2:46 p.m., Chug Rolke wrote:
> > /trunk/qpid/cpp/include/qpid/messaging/Logger.h, lines 83-84
> > <https://reviews.apache.org/r/16316/diff/1/?file=398786#file398786line83>
> >
> > "+" operates on the level and all higher levels
> > "-" operates on the level and all lower levels
Yep, this is better wording - thanks.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30530
-----------------------------------------------------------
On Dec. 17, 2013, 8:04 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 8:04 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551330
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request 16316: Implement control of internal logging in
qpid::messaging API
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16316/#review30530
-----------------------------------------------------------
Ship it!
Minor doc nits and some explanation for the wiki .
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58474>
"+" operates on the level and all higher levels
"-" operates on the level and all lower levels
/trunk/qpid/cpp/include/qpid/messaging/Logger.h
<https://reviews.apache.org/r/16316/#comment58475>
make one of these stderr
/trunk/qpid/cpp/src/qpid/log/Selector.cpp
<https://reviews.apache.org/r/16316/#comment58478>
Why can't a user disable critical level?
/trunk/qpid/cpp/src/qpid/messaging/Logger.cpp
<https://reviews.apache.org/r/16316/#comment58476>
How is this function's body not surrounded with {}? Seems unnatural.
- Chug Rolke
On Dec. 17, 2013, 8:04 a.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16316/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2013, 8:04 a.m.)
>
>
> Review request for qpid, Chug Rolke and Gordon Sim.
>
>
> Bugs: QPID-5415
> https://issues.apache.org/jira/browse/QPID-5415
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Currently there is one major omission from the qpid::messaging API: That is control of the internal logging used by qpid itself.
>
> This was previously present in the qpid::client API albeit not really supported or documented. Now that we have completely removed the qpid::client and associated APIs from being exported for use beyond qpid itself there is no way to control the logging internal to the qpid messaging libraries.
>
> This review introduces some simple APIs which give back control of the qpid's internal logging. I think there is now as much control as virtually all qpid user applications are likely to need.
>
> Please give the proposed API some consideration and let me know if there is anything that an application needs that I haven't considered here.
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Logger.h PRE-CREATION
> /trunk/qpid/cpp/src/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/qpid/log/Logger.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.h 1551330
> /trunk/qpid/cpp/src/qpid/log/Options.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Selector.cpp 1551330
> /trunk/qpid/cpp/src/qpid/log/Statement.cpp 1551330
> /trunk/qpid/cpp/src/qpid/messaging/Logger.cpp PRE-CREATION
> /trunk/qpid/cpp/src/tests/CMakeLists.txt 1551330
> /trunk/qpid/cpp/src/tests/MessagingLogger.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16316/diff/
>
>
> Testing
> -------
>
> Added in unit tests which pass*
>
> *They currently also assume a bug fix which is included in the code in this review which prevents critical log messages ever being turned off (I think this the correct behaviour - in the current code under some circumstances they can actually be turned off)
>
>
> Thanks,
>
> Andrew Stitcher
>
>