Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.99.0
    • Fix Version/s: 0.99.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      No important changes here, but I'm doing some other changes on top of this (typically HBASE-11297)

      Note that I've changed the logs, they now belong to the "real" class instead of hijacking Hadoop. I suppose it was historical, but it was as well very confusing.

      1. 11298.v1.patch
        27 kB
        Nicolas Liochon

        Issue Links

          Activity

          Hide
          Enis Soztutar added a comment -

          Closing this issue after 0.99.0 release.

          Show
          Enis Soztutar added a comment - Closing this issue after 0.99.0 release.
          Hide
          Nicolas Liochon added a comment -

          Included in HBASE-11297

          Show
          Nicolas Liochon added a comment - Included in HBASE-11297
          Hide
          Nicolas Liochon added a comment -

          I'm suspicious around the header...

          Show
          Nicolas Liochon added a comment - I'm suspicious around the header...
          Hide
          stack added a comment -

          ...on RpcServer, am I wrong or is maxIdleTime set to 2 seconds? My feeling is it's actually never used because we don't track the rpcCount correctly ?

          Yeah, looks like 2 seconds. How is it that we don't track rpcCount correctly? It is upped when we read a request and downed when we respond?

          Show
          stack added a comment - ...on RpcServer, am I wrong or is maxIdleTime set to 2 seconds? My feeling is it's actually never used because we don't track the rpcCount correctly ? Yeah, looks like 2 seconds. How is it that we don't track rpcCount correctly? It is upped when we read a request and downed when we respond?
          Hide
          Nicolas Liochon added a comment -

          Ok thanks, I can do that on commit.

          unrelated to this patch, but on RpcServer, am I wrong or is maxIdleTime set to 2 seconds? My feeling is it's actually never used because we don't track the rpcCount correctly ?

          Show
          Nicolas Liochon added a comment - Ok thanks, I can do that on commit. unrelated to this patch, but on RpcServer, am I wrong or is maxIdleTime set to 2 seconds? My feeling is it's actually never used because we don't track the rpcCount correctly ?
          Hide
          stack added a comment -

          Nicolas Liochon See here in the book http://hbase.apache.org/book.html#rpc.logging Look for org.apache.hadoop.ipc

          Show
          stack added a comment - Nicolas Liochon See here in the book http://hbase.apache.org/book.html#rpc.logging Look for org.apache.hadoop.ipc
          Hide
          Nicolas Liochon added a comment -

          Why we no longer need this: - if (inHandler) {

          It's always called with inHandler set to false. This code is as well refactored in HBASE-11297

          Oh, you made the RPC trace-level? Thats better (include doc change in your patch?)

          Yeah, I've done the cleanup (hopefully).
          I don't see a mention of "org.apache.hadoop.ipc.RpcServer" in the HBase book? May be somewhere else?

          On other hand this new method sets the buffer and authmethod... so maybe just leave as is and doc it sets these two data members on success.

          Yeah agreed.

          Show
          Nicolas Liochon added a comment - Why we no longer need this: - if (inHandler) { It's always called with inHandler set to false. This code is as well refactored in HBASE-11297 Oh, you made the RPC trace-level? Thats better (include doc change in your patch?) Yeah, I've done the cleanup (hopefully). I don't see a mention of "org.apache.hadoop.ipc.RpcServer" in the HBase book? May be somewhere else? On other hand this new method sets the buffer and authmethod... so maybe just leave as is and doc it sets these two data members on success. Yeah agreed.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12648353/11298.v1.patch
          against trunk revision .
          ATTACHMENT ID: 12648353

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12648353/11298.v1.patch against trunk revision . ATTACHMENT ID: 12648353 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9689//console This message is automatically generated.
          Hide
          Andrew Purtell added a comment -

          It's ok I can do the (partial)backport as you're ok for 98.

          Thanks. I'll wait for the final version rather than backport myself to try out HBASE-11297 and other related changes. However as soon as we have this issue committed in 0.98 I will gladly do that. I am about to start tracking down connection starvation issues under high load. I don't want to do that only to find results obviated by cleanups.

          Show
          Andrew Purtell added a comment - It's ok I can do the (partial)backport as you're ok for 98. Thanks. I'll wait for the final version rather than backport myself to try out HBASE-11297 and other related changes. However as soon as we have this issue committed in 0.98 I will gladly do that. I am about to start tracking down connection starvation issues under high load. I don't want to do that only to find results obviated by cleanups.
          Hide
          Nicolas Liochon added a comment -

          It's ok I can do the (partial)backport as you're ok for 98.

          Show
          Nicolas Liochon added a comment - It's ok I can do the (partial)backport as you're ok for 98.
          Hide
          stack added a comment -

          Note that I've changed the logs, they now belong to the "real" class instead of hijacking Hadoop.

          RPC DEBUG is verbose, more verbose than all other hbase DEBUG logging so much so that it overwhelms. Above is an old means of enabling RPC DEBUG independent of DEBUG level everywhere else in HBASE. We'd doc'd it here: http://hbase.apache.org/book.html#rpc.logging

          Maybe not INFO level is default and RPC logging has had an edit, it might be OK making it so DEBUG enable'd RPC logging..... have you checked?

          Oh, you made the RPC trace-level? Thats better (include doc change in your patch?)

          Why we no longer need this: - if (inHandler) {

          Should you pass in the buffer readPreamble works on rather than rely on 'this'? I don't like methods that have side effects; i.e. setting 'this' items if it returns successufully. On other hand this new method sets the buffer and authmethod... so maybe just leave as is and doc it sets these two data members on success.

          Otherwise, cleanup looks good to me. +1 to commit if hadoopqa doesn't burp.

          Show
          stack added a comment - Note that I've changed the logs, they now belong to the "real" class instead of hijacking Hadoop. RPC DEBUG is verbose, more verbose than all other hbase DEBUG logging so much so that it overwhelms. Above is an old means of enabling RPC DEBUG independent of DEBUG level everywhere else in HBASE. We'd doc'd it here: http://hbase.apache.org/book.html#rpc.logging Maybe not INFO level is default and RPC logging has had an edit, it might be OK making it so DEBUG enable'd RPC logging..... have you checked? Oh, you made the RPC trace-level? Thats better (include doc change in your patch?) Why we no longer need this: - if (inHandler) { Should you pass in the buffer readPreamble works on rather than rely on 'this'? I don't like methods that have side effects; i.e. setting 'this' items if it returns successufully. On other hand this new method sets the buffer and authmethod... so maybe just leave as is and doc it sets these two data members on success. Otherwise, cleanup looks good to me. +1 to commit if hadoopqa doesn't burp.
          Hide
          Andrew Purtell added a comment -

          +1

          I would leave this hunk out of a 0.98 patch:

          --- hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          +++ hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java
          @@ -144,9 +144,7 @@ import com.google.protobuf.TextFormat;
            */
           @InterfaceAudience.Private
           public class RpcServer implements RpcServerInterface {
          -  // The logging package is deliberately outside of standard o.a.h.h package so it is not on
          -  // by default.
          -  public static final Log LOG = LogFactory.getLog("org.apache.hadoop.ipc.RpcServer");
          +  public static final Log LOG = LogFactory.getLog(RpcServer.class);
           
             private final boolean authorize;
             private boolean isSecurityEnabled;
          
          Show
          Andrew Purtell added a comment - +1 I would leave this hunk out of a 0.98 patch: --- hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java +++ hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java @@ -144,9 +144,7 @@ import com.google.protobuf.TextFormat; */ @InterfaceAudience.Private public class RpcServer implements RpcServerInterface { - // The logging package is deliberately outside of standard o.a.h.h package so it is not on - // by default . - public static final Log LOG = LogFactory.getLog( "org.apache.hadoop.ipc.RpcServer" ); + public static final Log LOG = LogFactory.getLog(RpcServer.class); private final boolean authorize; private boolean isSecurityEnabled;
          Hide
          Andrew Purtell added a comment -

          Please consider 0.98 also. Or I could do a back port.

          Show
          Andrew Purtell added a comment - Please consider 0.98 also. Or I could do a back port.

            People

            • Assignee:
              Nicolas Liochon
              Reporter:
              Nicolas Liochon
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development