qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
Subject Re: Review Request 16316: Implement control of internal logging in qpid::messaging API
Date Fri, 03 Jan 2014 14:39:41 GMT


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


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