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