[
https://issues.apache.org/jira/browse/HDFS-9118?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15198029#comment-15198029
]
Bob Hansen commented on HDFS-9118:
----------------------------------
James - thanks for carrying that forward and festooning our code with
appropriate messages. It has already proven useful.
The current implementation always constructs a LogMessage but turns the <<
operators into no-ops if the logging level is too low to be displayed. The
downside of this approach is that all of the functions calls that construct the
log message will still be called and construct return strings only to throw
them away. We tried to come up with a clever templating method to get rid of
that, but I think we should still take it on as a use-case and solve with
macros. If we construct the logging macros as:
#define LOG_TRACE (C,MSG) { if (LogManager::ShouldLog(kTrace ,C)) {
LogMessage(kTrace ) MSG; } }
#define LOG_DEBUG (C,MSG) { if (LogManager::ShouldLog(kDebug ,C)) {
LogMessage(kDebug ) MSG; } }
#define LOG_INFO (C,MSG) { if (LogManager::ShouldLog(kInfo ,C)) {
LogMessage(kInfo ) MSG; } }
#define LOG_WARNING(C,MSG) { if (LogManager::ShouldLog(kWarning,C)) {
LogMessage(kWarning) MSG; } }
#define LOG_ERROR (C,MSG) { if (LogManager::ShouldLog(kError ,C)) {
LogMessage(kError ) MSG; } }
The consumer code looks like:
LOG_WARNING(kRPC, << "RPC response with Unknown call id " << h.callid());
This gives us the advantage of clean code but skips the performance hit of
constructing useless data.
LoggerIntf::SetLogLevel:
std::cerr << ....should probably be removed
Does logging.h require hdfspp/hdfs_ext.h for anything other than the LogData
declaration (used only by the CForwardingLogger)?
Can we move the CForwardingLogger into hdfs.cc? Since it's C-API-specific, it
seems to be more tightly bound there than in the logging source file.
In the static log manager methods, we make calls to the logger_impl, calls that
are explicitly forbidden from being virtual. While I'm not keen on that design
choice (premature optimization and all that), why not just put the variables
and code right in the LogManager. Implementations can't override the semantics
of the calls, so it doesn't appear that we're not gaining any flexibility by
dereferencing the logger_impl, and there will some (minimal) runtime impact
(along with increasing the size and complexity of the code). The cleans up the
LoggerInterface to a single pure virtual method (which is very nice).
I would nix the GET_IMPL_LOCK macro and just copy/paste the code in the few
places where it needs to be. More explict that way, at the cost of evil
copypasta.
Upon reflection, do we have a strong use case for having different log levels
for different components, and enabling and disabling by component? Marking
with a component for identification I can understand.
>From hdfs_ext.h:
* Unlike the C++ logger this will not filter by level or component,
* it is up to the consumer to throw away messages they don't want.
*
* Note: The callback provided must be reentrant, the library does not
guarentee
* that there won't be concurrent calls.
By my reding, the log currenlty implemented log manager does provide both of
those guarantees to the CForwardingLogger.
Should the CForwardingLogger have a const * to the callback set in the ctor
rather than a setter? We could then get rid of the !callback_ checks.
Should LogMessage& LogMessage::operator<<(const std::string *str) log "nullptr"
if str is null?
In logging.h, we've commented out:
//std::cerr << "msg ctor called, worth_reporting_=" << worth_reporting_
// << "level = " << level_string() << ", component=" <<
component_string() << std::endl;
It should be removed.
Can we tie ReportError in hdfs.cc to the logging system?
> Add logging system for libdhfs++
> --------------------------------
>
> Key: HDFS-9118
> URL: https://issues.apache.org/jira/browse/HDFS-9118
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Affects Versions: HDFS-8707
> Reporter: Bob Hansen
> Assignee: James Clampffer
> Attachments: HDFS-9118.HDFS-8707.000.patch,
> HDFS-9118.HDFS-8707.001.patch, HDFS-9118.HDFS-8707.002.patch
>
>
> With HDFS-9505, we've starting logging data from libhdfs++. Consumers of the
> library are going to have their own logging infrastructure that we're going
> to want to provide data to.
> libhdfs++ should have a logging library that:
> * Is overridable and can provide sufficient information to work well with
> common C++ logging frameworks
> * Has a rational default implementation
> * Is performant
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
