[
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.
{quote}
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.
{quote}
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.
{quote}
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).
{quote}
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.
{quote}
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.
{quote}
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.
{quote}
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.
{quote}
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 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)
