hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Clampffer (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-9118) Add logging system for libdhfs++
Date Wed, 16 Mar 2016 21:19:33 GMT

    [ https://issues.apache.org/jira/browse/HDFS-9118?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15198193#comment-15198193

James Clampffer commented on HDFS-9118:

bq. 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
I agree. I just did some profiling today and a decent chunk of time gets spent constructing
std::stringstream owned by LogMessage, so that needs to be fixed.

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.
I also needs the #defines.  I can separate out a header and have hdfs_ext.h and logging.h
both include that to decouple things.  I can also move the CForwardingLogger.

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).
Good point, I was going to move them into LogManager in the last round and I guess I forgot.

bq. 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.
Sure, I think this was another remnant of when I had a lot more methods getting locks or something
like that.  Not really that useful now.

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.
My main use case is for debugging, and I've been doing a lot of that.  I've used it quite
a bit lately to focus on what could be a nasty bug in the RPC engine; it lets me avoid grepping
through the logs.  It's also really inexpensive to do so I don't see it hurting anything.
 Could always mark it as unsupported or something like that.

bq. By my reading, the log currenlty implemented log manager does provide both of those guarantees
to the CForwardingLogger.
At the moment it does do both of those.  In an earlier design I had it didn't but I left those
comments to leave some room in case I wanted to change things around.  I can strip them out
but I don't see making the interface slightly more restrictive for the client and implementation
more flexible as being too bad of a trade.

bq. 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.
It wouldn't hurt to have the setter create a new CForwardingLogger when the client sets a
new callback.  The main reason I did this is so that I can pass a null pointer to flush out
the old callback and disable logging.  I'm not a big fan of having to define no-op callbacks,
particularly for stuff with weird signatures, to get that same effect.  I'm not too concerned
about the runtime cost of the check (branch should be super predictable).  My preference would
be to keep it how it is.

bq. Should LogMessage& LogMessage::operator<<(const std::string *str) log "nullptr"
if str is null?
Yea, that's a good point.  I'll change that.  Will do the same for the const char * version
as well.

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.
Good catch, will fix that and the write to cerr you caught above.

bq. Can we tie ReportError in hdfs.cc to the logging system?
For sure, that would make things really clean.  I'd like to push that into another jira just
so I can land this soon and minimize how much I have to rebase before I do though.

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

View raw message