hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bob Hansen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-9118) Add logging system for libdhfs++
Date Tue, 22 Mar 2016 15:19:25 GMT

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

Bob Hansen commented on HDFS-9118:
----------------------------------

I like the look of that patch, [~James Clampffer].

A few more comments to take or leave as you choose:
* 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.

* Should LogData be part of hdfspp_ext.h rather than log.h?  It seems to be specific to the
CForwardingLogger

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

* 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

* We might want to move ShouldLog into the header so it can be inlined

* Is there a reason for the two distinct .reset calls in ::SetLoggerImpl rather than just
one?

* std::asctime is deprecated and not thread-safe.  We should use std::strftime which is less
straightforward but safer

* 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

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

* 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

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

* I think FileHandler::CancelOperations should be at the Info level

None of those are show-stoppers, but I think some of them would make the first pass a bit
better.



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

Mime
View raw message