HBase
  1. HBase
  2. HBASE-7097

Log message in SecureServer.class uses wrong class name

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.92.2, 0.94.2
    • Fix Version/s: 0.92.3, 0.94.3
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      log messages are printing wrongly in org.apache.hadoop.hbase.ipc.SecureServer.class

      public static final Log LOG = LogFactory.getLog("org.apache.hadoop.ipc.SecureServer"); 
        private static final Log AUDITLOG = 
          LogFactory.getLog("SecurityLogger.org.apache.hadoop.ipc.SecureServer"); 
      
      1. HBASE-7097-addendum.patch
        6 kB
        Andrew Purtell
      2. HBASE-7097-addendum.patch
        6 kB
        Andrew Purtell
      3. HBASE-7097_94.patch
        1.0 kB
        Y. SREENIVASULU REDDY

        Activity

        Hide
        Y. SREENIVASULU REDDY added a comment -

        attached patch for 94 branch.

        Show
        Y. SREENIVASULU REDDY added a comment - attached patch for 94 branch.
        Hide
        Andrew Purtell added a comment -

        +1, looks like a remnant of the port from Hadoop that wasn't caught on code review.

        Show
        Andrew Purtell added a comment - +1, looks like a remnant of the port from Hadoop that wasn't caught on code review.
        Hide
        Lars Hofhansl added a comment -

        +1

        Show
        Lars Hofhansl added a comment - +1
        Hide
        Andrew Purtell added a comment -

        Committed to 0.94 and 0.92 branches. Thanks for the patch. Thanks for the ack on 0.94 Lars.

        Show
        Andrew Purtell added a comment - Committed to 0.94 and 0.92 branches. Thanks for the patch. Thanks for the ack on 0.94 Lars.
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #574 (See https://builds.apache.org/job/HBase-0.94/574/)
        HBASE-7097. Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #574 (See https://builds.apache.org/job/HBase-0.94/574/ ) HBASE-7097 . Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906) Result = FAILURE apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #604 (See https://builds.apache.org/job/HBase-0.92/604/)
        HBASE-7097. Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405909)

        Result = SUCCESS
        apurtell :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #604 (See https://builds.apache.org/job/HBase-0.92/604/ ) HBASE-7097 . Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405909) Result = SUCCESS apurtell : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Gary Helmling added a comment -

        This was unfortunately intentional, at least in the LOG instance, in order to provide an admittedly cludgey way to enable debug logging for the entire org.apache.hadoop.hbase logger without spamming region server logs with per-request messages – per request metrics only get logged if you enable debug logging for org.apache.hadoop.ipc. It preserves the same technique that is used in HBaseServer.

        No argument from me that the technique is ugly. But if we actually fix the logger to use the correct class name, we should also change per-request logging to only happen at trace level instead of debug, so that existing HBase setups with debug level enabled for the org.apache.hadoop.hbase parent don't start getting noisy per-request messages.

        Show
        Gary Helmling added a comment - This was unfortunately intentional, at least in the LOG instance, in order to provide an admittedly cludgey way to enable debug logging for the entire org.apache.hadoop.hbase logger without spamming region server logs with per-request messages – per request metrics only get logged if you enable debug logging for org.apache.hadoop.ipc. It preserves the same technique that is used in HBaseServer. No argument from me that the technique is ugly. But if we actually fix the logger to use the correct class name, we should also change per-request logging to only happen at trace level instead of debug, so that existing HBase setups with debug level enabled for the org.apache.hadoop.hbase parent don't start getting noisy per-request messages.
        Hide
        Andrew Purtell added a comment -

        But if we actually fix the logger to use the correct class name, we should also change per-request logging to only happen at trace level instead of debug, so that existing HBase setups with debug level enabled for the org.apache.hadoop.hbase parent don't start getting noisy per-request messages.

        +1 here, can do an addendum

        Show
        Andrew Purtell added a comment - But if we actually fix the logger to use the correct class name, we should also change per-request logging to only happen at trace level instead of debug, so that existing HBase setups with debug level enabled for the org.apache.hadoop.hbase parent don't start getting noisy per-request messages. +1 here, can do an addendum
        Hide
        Lars Hofhansl added a comment -

        Maybe we should roll this back, at least for 0.94?

        Show
        Lars Hofhansl added a comment - Maybe we should roll this back, at least for 0.94?
        Hide
        Andrew Purtell added a comment - - edited

        Maybe we should roll this back, at least for 0.94?

        And we will do it again in 6 months when it gets pointed out again and this detail has slipped cache again of the first wanderers by? Better to finish with the proposed addendum I think.

        Edit: Either way we need an addendum, if rolling back we need a comment added as explanation.

        Show
        Andrew Purtell added a comment - - edited Maybe we should roll this back, at least for 0.94? And we will do it again in 6 months when it gets pointed out again and this detail has slipped cache again of the first wanderers by? Better to finish with the proposed addendum I think. Edit: Either way we need an addendum, if rolling back we need a comment added as explanation.
        Hide
        Lars Hofhansl added a comment -

        Sure... We can change the per request logging to TRACE too.

        Show
        Lars Hofhansl added a comment - Sure... We can change the per request logging to TRACE too.
        Hide
        Andrew Purtell added a comment -

        Sure... We can change the per request logging to TRACE too.

        I'll wait for a bit to see if there's more opinion, otherwise will do so.

        Show
        Andrew Purtell added a comment - Sure... We can change the per request logging to TRACE too. I'll wait for a bit to see if there's more opinion, otherwise will do so.
        Hide
        Gary Helmling added a comment -

        +1 to an addendum to change the per-request logging to trace level. Works for me.

        Show
        Gary Helmling added a comment - +1 to an addendum to change the per-request logging to trace level. Works for me.
        Hide
        Lars Hofhansl added a comment -

        I'm also fine with reverting and just adding a comment. This class is gone in 0.96, so when 0.94 is eventually phased out this is no longer a problem anyway.

        Either way, make an addendum Andy?

        +1 on changing all debug logs using this logger to trace,
        alternatively
        +1 on reverting with comment

        Show
        Lars Hofhansl added a comment - I'm also fine with reverting and just adding a comment. This class is gone in 0.96, so when 0.94 is eventually phased out this is no longer a problem anyway. Either way, make an addendum Andy? +1 on changing all debug logs using this logger to trace, alternatively +1 on reverting with comment
        Hide
        Andrew Purtell added a comment -

        Proposed addendum attached. Any more of the DEBUG level messages should go to TRACE?

        Show
        Andrew Purtell added a comment - Proposed addendum attached. Any more of the DEBUG level messages should go to TRACE?
        Hide
        Lars Hofhansl added a comment -

        I think this looks reasonable.
        Should we leave the "SecurityEnabled" message?

        I have no good sense about how frequently

                    if (LOG.isDebugEnabled())
                      LOG.debug("Created SASL server with mechanism = "
                          + authMethod.getMechanismName());
        

        or

                     if (LOG.isDebugEnabled())
                        LOG.debug("Kerberos principal name is " + fullName);
        

        Would called, but I think it should be OK to leave those at DEBUG.

        Show
        Lars Hofhansl added a comment - I think this looks reasonable. Should we leave the "SecurityEnabled" message? I have no good sense about how frequently if (LOG.isDebugEnabled()) LOG.debug( "Created SASL server with mechanism = " + authMethod.getMechanismName()); or if (LOG.isDebugEnabled()) LOG.debug( "Kerberos principal name is " + fullName); Would called, but I think it should be OK to leave those at DEBUG.
        Hide
        Andrew Purtell added a comment -

        Thanks for taking a look Lars. I'll commit the proposed addendum with the additional log messages you called out moved to TRACE level shortly unless there is further comment. I don't think they are that useful.

        Show
        Andrew Purtell added a comment - Thanks for taking a look Lars. I'll commit the proposed addendum with the additional log messages you called out moved to TRACE level shortly unless there is further comment. I don't think they are that useful.
        Hide
        Lars Hofhansl added a comment -

        +1

        Show
        Lars Hofhansl added a comment - +1
        Hide
        Andrew Purtell added a comment -

        Committed addendum to 0.94 and 0.92 branches.

        Show
        Andrew Purtell added a comment - Committed addendum to 0.94 and 0.92 branches.
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #575 (See https://builds.apache.org/job/HBase-0.94/575/)
        Amend HBASE-7097. Change per-request logging in SecureServer to TRACE level (Revision 1406420)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #575 (See https://builds.apache.org/job/HBase-0.94/575/ ) Amend HBASE-7097 . Change per-request logging in SecureServer to TRACE level (Revision 1406420) Result = FAILURE apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #605 (See https://builds.apache.org/job/HBase-0.92/605/)
        Amend HBASE-7097. Change per-request logging in SecureServer to TRACE level (Revision 1406421)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #605 (See https://builds.apache.org/job/HBase-0.92/605/ ) Amend HBASE-7097 . Change per-request logging in SecureServer to TRACE level (Revision 1406421) Result = FAILURE apurtell : Files : /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #83 (See https://builds.apache.org/job/HBase-0.94-security/83/)
        Amend HBASE-7097. Change per-request logging in SecureServer to TRACE level (Revision 1406420)
        HBASE-7097. Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906)

        Result = SUCCESS
        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java

        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #83 (See https://builds.apache.org/job/HBase-0.94-security/83/ ) Amend HBASE-7097 . Change per-request logging in SecureServer to TRACE level (Revision 1406420) HBASE-7097 . Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906) Result = SUCCESS apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #10 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/10/)
        Amend HBASE-7097. Change per-request logging in SecureServer to TRACE level (Revision 1406420)
        HBASE-7097. Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java

        apurtell :
        Files :

        • /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #10 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/10/ ) Amend HBASE-7097 . Change per-request logging in SecureServer to TRACE level (Revision 1406420) HBASE-7097 . Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405906) Result = FAILURE apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java apurtell : Files : /hbase/branches/0.94/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92-security #147 (See https://builds.apache.org/job/HBase-0.92-security/147/)
        Amend HBASE-7097. Change per-request logging in SecureServer to TRACE level (Revision 1406421)
        HBASE-7097. Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405909)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java

        apurtell :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java
        Show
        Hudson added a comment - Integrated in HBase-0.92-security #147 (See https://builds.apache.org/job/HBase-0.92-security/147/ ) Amend HBASE-7097 . Change per-request logging in SecureServer to TRACE level (Revision 1406421) HBASE-7097 . Log message in SecureServer.class uses wrong class name (Y. Sreenivasulu Reddy) (Revision 1405909) Result = FAILURE apurtell : Files : /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java apurtell : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/security/src/main/java/org/apache/hadoop/hbase/ipc/SecureServer.java

          People

          • Assignee:
            Unassigned
            Reporter:
            Y. SREENIVASULU REDDY
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development