Mailing-List: contact hdfs-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: hdfs-issues@hadoop.apache.org
Date: Tue, 22 Mar 2016 15:19:25 +0000 (UTC)
From: "Bob Hansen (JIRA)" <jira@apache.org>
To: hdfs-issues@hadoop.apache.org
Message-ID: <JIRA.12895242.1442866186000.17943.1458659965496@Atlassian.JIRA>
In-Reply-To: <JIRA.12895242.1442866186000@Atlassian.JIRA>
References: <JIRA.12895242.1442866186000@Atlassian.JIRA>
 <JIRA.12895242.1442866186800@arcas>
Subject: [jira] [Commented] (HDFS-9118) Add logging system for libdhfs++
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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