[
https://issues.apache.org/jira/browse/HDFS-9118?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15208701#comment-15208701
]
James Clampffer commented on HDFS-9118:
---------------------------------------
Didn't see you had more comments above my last message, thanks for the very
detailed feedback, let me give those a shot.
bq. If log.h is going to be included in libhdfspp_ext, it should have its own
extern "C" blocks to make sure that C++ idioms don't creep in.
Good idea. I should be able to add this.
bq. Should LogData be part of hdfspp_ext.h rather than log.h? It seems to be
specific to the CForwardingLogger
Most likely. I can't remember offhand I left it in there to break up some
dependency issues or I just forgot about it. If it's the latter I'll move it.
bq. In isComponentValid/isLogLevel valid, we should declare a MAX_LOG_LEVEL and
MAX_COMPONENT in log.h so that it is tied in code closer to where it is
declared.
I can do this if I'm in the area while fixing other things. I don't see it as
being too critical in the short term and I think longer term the #defines
should get replaced with C enums, but that's a bunch of work to do and I'd like
to get this in fairly soon since it's proven useful as-is.
bq. Are we allowing multiple components to be specified in enableComponent,
disableComponent? If so, the upper limit on our bounds check should be
(MAX_COMPONENT << 1) - 1. If not, we should check that only one bit is set in
isComponentValid
It's intended to allow only 1 component at a time, I can add a popcount and
error/no-op if >1 bits are set.
bq. We might want to move ShouldLog into the header so it can be inlined
This is a good point, I was hoping -flto would be smart enough to handle this
but a quick look says support varies a lot by platform even with simple
functions.
bq. Is there a reason for the two distinct .reset calls in ::SetLoggerImpl
rather than just one?
The compiler was being grumpy when I first wrote it as a single one (I forget
why) so I just wrote two and moved on. I'll take a look at this, but if it
looks like it will take a significant amount of time to figure out while the
compiler is complaining I'm just going to leave it; it's pretty understandable
and is nearly free.
bq. std::asctime is deprecated and not thread-safe. We should use std::strftime
which is less straightforward but safer
I'll check this out, didn't realize it wasn't thread safe. May hold off on
this until the implementation changes enough that the logger plugins really can
get called by multiple threads.
bq. For null pointer output, as a consumer I would prefer to see just "nullptr"
or "NULL" rather than including the type of the null pointer in the output
Hm. This might be a matter of preference; I wanted to know the type in case it
could have been ambiguous. If adding the __FILE__/__LINE__ stuff looks like it
will disambiguate in most cases I'll get rid of the type.
bq. As part of HDFS-9616, I've added the cluster and filename to the relevant
objects. We should follow-up and either add them to the LogMessage
macros/object or to the output messages.
I'm going to leave this out for now to prevent the scope of this patch from
creeping too much. It seems like it would be fitting to do this when
integrating the C API ReportError with the logging system.
bq. In the logging test, it is good form to have each test set itself up and
tear itself down rather than putting the setup code in main. Either make it a
class with a SetUp method or add an RAII object to the top of each test to do
the register/unregister
This was done to minimize boilerplate and definitions of single use objects. I
think the current implementation is maintainable as-is, but if much more stuff
is added the cost of defining RAII objects would be amortized enough that I
think it'd be good to have.
{quote}
As a consumer, I would like to see more information in the Debug level between
Trace and Info.
All object's ctors and dtors (with "this" pointer)
Anything that happens more than once-per-file but less than once-per-block. I
might suggest:
FileHandleImpl::PositionRead
FileHandleImpl::Read
FileHandleImpl::Seek
Should we include the per-block operations (past BlockReader ctor/dtor)?
Anything else that's interesting here?
{quote}
(Not sure how to maintain list formatting in quotes..)
In order do track all ctor/dtor calls I'd like to make some RAII member object
that takes up no space in the class to handle it. I'll take a look at what I
can do with some macros, if it looks like it will be a pain I'm going to push
it off into another improvement jira. What I want is something like:
{code}
// Drop this into any class, will determine class name at build time (somehow,
doesn't look like __CLASS__ exists)
// and print class + this* on construction and destruction (instantiate some
RAII object).
#define TRACK_OBJECT_LIFETIME ... some stuff
class RpcEngineOrAnyOtherClass {
public:
TRACK_OBJECT_LIFETIME //clean call site
public:
methods and stuff
};
{code}
I'll most likely add the handling for file open/closes in the next patch. My
hope is that once this is landed people add their own logging to parts as they
see fit, I want to just get in enough to show how it can be done and cover the
most useful (error prone) cases.
bq. I think FileHandler::CancelOperations should be at the Info level
Sure, easy enough to change.
> 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,
> HDFS-9118.HDFS-8707.003.patch, HDFS-9118.HDFS-8707.003.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)
