HBase
  1. HBase
  2. HBASE-5045

Add the table name and cf name for the next call int the task monitor

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      In the task monitor, we don't have much information about the next call compared to other operations.
      It would be nice to add the table name and cf name for each next call in the task monitor.

      1. ASF.LICENSE.NOT.GRANTED--D3045.3.patch
        38 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--D3045.2.patch
        38 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--D3045.1.patch
        38 kB
        Phabricator
      4. ASF.LICENSE.NOT.GRANTED--D2913.2.patch
        32 kB
        Phabricator
      5. ASF.LICENSE.NOT.GRANTED--D2913.1.patch
        31 kB
        Phabricator

        Activity

        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".
        Reviewers: amirshim, Liyin, tedyu, stack, JIRA

        Porting Amir's fix from 89-fb. His original summary below.

        A method for associating pretty print classes with method calls.
        These allow you to get information about a method call given the params it was
        called with and what instance it was called on.

        The first use case is for getting info about a next() RPC call.

        TEST PLAN
        Original test plan from Amir: run a script that stresses a regionserver with scan and next() scans, and check that the information is show in the JSON view of the TaskMonitor

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/6645/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Reviewers: amirshim, Liyin, tedyu, stack, JIRA Porting Amir's fix from 89-fb. His original summary below. A method for associating pretty print classes with method calls. These allow you to get information about a method call given the params it was called with and what instance it was called on. The first use case is for getting info about a next() RPC call. TEST PLAN Original test plan from Amir: run a script that stresses a regionserver with scan and next() scans, and check that the information is show in the JSON view of the TaskMonitor REVISION DETAIL https://reviews.facebook.net/D2913 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6645/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".
        Reviewers: amirshim, Liyin, tedyu, stack, JIRA

        Fixing a bug in my port (not present in the original patch) that broke RPC.

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Reviewers: amirshim, Liyin, tedyu, stack, JIRA Fixing a bug in my port (not present in the original patch) that broke RPC. REVISION DETAIL https://reviews.facebook.net/D2913 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        WritableRpcEngine.java is going to be replaced in trunk. Would another issue be filed to make this work with PB-based RPC ?

        All the catch clause should be on the same line as the preceding closing brace.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:193 'a RPC call' -> 'an RPC call'
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java:22 This import should be after line 27.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3488 This should be package private.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2217 Either return res here or move line 2216 after this line.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2206 '1 and 2 parameter' -> '1-parameter and 2-parameter'
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2222 Can getOriginalScan() be declared in RegionScanner ?
        It doesn't look nice referencing an implementation class.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:2 No year is needed.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:31 DEFAULT can be detailed, right ?
        How about naming DEFAULT CONCISE ?
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:2 No year, please.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:72 Insert a space after catch
        Put this on line 71.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:77 Put this on line 76.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:74 Insert a comma before the space of " make sure"
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:2 No year, please.
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:1 License, please.
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:16 Add test category.

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        Show
        Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". WritableRpcEngine.java is going to be replaced in trunk. Would another issue be filed to make this work with PB-based RPC ? All the catch clause should be on the same line as the preceding closing brace. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:193 'a RPC call' -> 'an RPC call' src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java:22 This import should be after line 27. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3488 This should be package private. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2217 Either return res here or move line 2216 after this line. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2206 '1 and 2 parameter' -> '1-parameter and 2-parameter' src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2222 Can getOriginalScan() be declared in RegionScanner ? It doesn't look nice referencing an implementation class. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:2 No year is needed. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:31 DEFAULT can be detailed, right ? How about naming DEFAULT CONCISE ? src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:2 No year, please. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:72 Insert a space after catch Put this on line 71. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:77 Put this on line 76. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:74 Insert a comma before the space of " make sure" src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:2 No year, please. src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:1 License, please. src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:16 Add test category. REVISION DETAIL https://reviews.facebook.net/D2913
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12523605/D2913.2.patch
        against trunk revision .

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

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

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

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

        -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings.

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

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//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/12523605/D2913.2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1597//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        nspiegelberg has accepted the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        lgtm. should see what minor nits are applicable and fix them. this has already been reviewed internally and was a trivial port.

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        BRANCH
        add_the_table_name_and_cf_name_for_the_next_HBASE-5045_v3

        Show
        Phabricator added a comment - nspiegelberg has accepted the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". lgtm. should see what minor nits are applicable and fix them. this has already been reviewed internally and was a trivial port. REVISION DETAIL https://reviews.facebook.net/D2913 BRANCH add_the_table_name_and_cf_name_for_the_next_ HBASE-5045 _v3
        Hide
        Phabricator added a comment -

        stack has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        This addition, while I'm sure its sweet, is tough to follow. The model needs tweaking IMO.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java:51 Is there an actual change here? If there is, the extra import doesn't do anything it seems.
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:197 What does this method do? Return a map keyed by what? What is the String? Where would I plug this in or how would I use it? Does an inspecific method like this need to be public?
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java:319 Is there reason for flipping order in which these methods are called or is it just vagaries of patch (0.89fb vs trunk)?
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java:45 Whats a 'realMethod'?
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:86 gratuitous change
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3489 Does this have to public?

        Why is this 'originalScan' rather than just 'scan'?
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2210 Does this inner class have to be in HRS? Its a massive class already.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2221 Do we have to reach into an HRegion like this? Can we not use an accessor? This can be brittle going forward.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:2 No need of this and its wrong year anyways.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:27 Should we start an annotations package? There are other annotations floating about hbase.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:41 This comment is about the future? What is this method doing now?
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:2 ditto w/ above
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:63 A method named getMap seems way too broad a target to find using introspection. I'd think we'd make the target narrow so for sure we returned only when we had the right method.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:79 Ok to ignore?
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:102 This method is a pretty print of a method invocation? Should we call it that instead instead of a getMap? getMap is generic. If it were called prettyPrint, my guess is it'd be clear to most whats going on here.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:2 ditto

        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:24 Should it be called ParamPrettyPrinter or MethodPrettyPrinter or MethodInvocationPrettyPrinter instead?
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:32 See comments above

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        BRANCH
        add_the_table_name_and_cf_name_for_the_next_HBASE-5045_v3

        Show
        Phabricator added a comment - stack has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". This addition, while I'm sure its sweet, is tough to follow. The model needs tweaking IMO. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java:51 Is there an actual change here? If there is, the extra import doesn't do anything it seems. src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:197 What does this method do? Return a map keyed by what? What is the String? Where would I plug this in or how would I use it? Does an inspecific method like this need to be public? src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java:319 Is there reason for flipping order in which these methods are called or is it just vagaries of patch (0.89fb vs trunk)? src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java:45 Whats a 'realMethod'? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:86 gratuitous change src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3489 Does this have to public? Why is this 'originalScan' rather than just 'scan'? src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2210 Does this inner class have to be in HRS? Its a massive class already. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2221 Do we have to reach into an HRegion like this? Can we not use an accessor? This can be brittle going forward. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:2 No need of this and its wrong year anyways. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:27 Should we start an annotations package? There are other annotations floating about hbase. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:41 This comment is about the future? What is this method doing now? src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:2 ditto w/ above src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:63 A method named getMap seems way too broad a target to find using introspection. I'd think we'd make the target narrow so for sure we returned only when we had the right method. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:79 Ok to ignore? src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:102 This method is a pretty print of a method invocation? Should we call it that instead instead of a getMap? getMap is generic. If it were called prettyPrint, my guess is it'd be clear to most whats going on here. src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:2 ditto src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:24 Should it be called ParamPrettyPrinter or MethodPrettyPrinter or MethodInvocationPrettyPrinter instead? src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:32 See comments above REVISION DETAIL https://reviews.facebook.net/D2913 BRANCH add_the_table_name_and_cf_name_for_the_next_ HBASE-5045 _v3
        Hide
        Phabricator added a comment -

        amirshim has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java:319 status.setRPC a couple of lines away now takes method as a param.
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java:45 The "real" method returned by the Java reflection API... see MonitoredRPCHandlerImpl for more info.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3489 This object is stored across RPC calls and we store the original scan, so that we can easily print the information about the scan in the task monitor. It needs to be accessible from ScanParamsFormatter
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2210 It doesn't have to be in here, but if not, then we have to expose some internals of HRS. This class needs to understand the internals, since it provides introspection about what it's method(s) do. i.e. If we change how we do scanner lookups, this class needs to change too.
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2221 It's supposed to be strongly bound since it provides introspection about it's methods.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:27 It's possible, but should be done in another diff.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:63 It's introspection on a method we control, since we force it to derive from ParamFormatter<>. I called it getMap() to be consistent with the getMap() used in the TaskMonitor.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:102 See above. I called it getMap to be consistent with the TaskMonitor functions.
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:24 It formats information about a method call given the params it takes. PrettyPrint makes me think that it actually outputs the info somewhere, as opposed to a formatter that arranges the data for later pretty printing. I shouldn't have put pretty print in so many comments.

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        BRANCH
        add_the_table_name_and_cf_name_for_the_next_HBASE-5045_v3

        Show
        Phabricator added a comment - amirshim has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java:319 status.setRPC a couple of lines away now takes method as a param. src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java:45 The "real" method returned by the Java reflection API... see MonitoredRPCHandlerImpl for more info. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:3489 This object is stored across RPC calls and we store the original scan, so that we can easily print the information about the scan in the task monitor. It needs to be accessible from ScanParamsFormatter src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2210 It doesn't have to be in here, but if not, then we have to expose some internals of HRS. This class needs to understand the internals, since it provides introspection about what it's method(s) do. i.e. If we change how we do scanner lookups, this class needs to change too. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java:2221 It's supposed to be strongly bound since it provides introspection about it's methods. src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java:27 It's possible, but should be done in another diff. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:63 It's introspection on a method we control, since we force it to derive from ParamFormatter<>. I called it getMap() to be consistent with the getMap() used in the TaskMonitor. src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java:102 See above. I called it getMap to be consistent with the TaskMonitor functions. src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java:24 It formats information about a method call given the params it takes. PrettyPrint makes me think that it actually outputs the info somewhere, as opposed to a formatter that arranges the data for later pretty printing. I shouldn't have put pretty print in so many comments. REVISION DETAIL https://reviews.facebook.net/D2913 BRANCH add_the_table_name_and_cf_name_for_the_next_ HBASE-5045 _v3
        Hide
        Phabricator added a comment -

        amirshim requested code review of "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".
        Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg

        Porting Amir's fix from 89-fb. His original summary below.

        A method for associating pretty print classes with method calls.
        These allow you to get information about a method call given the params it was
        called with and what instance it was called on.

        The first use case is for getting info about a next() RPC call.

        TEST PLAN
        Original test plan from Amir: run a script that stresses a
        regionserver with scan and next() scans, and check that the information is show
        in the JSON view of the TaskMonitor

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/6915/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - amirshim requested code review of " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg Porting Amir's fix from 89-fb. His original summary below. A method for associating pretty print classes with method calls. These allow you to get information about a method call given the params it was called with and what instance it was called on. The first use case is for getting info about a next() RPC call. TEST PLAN Original test plan from Amir: run a script that stresses a regionserver with scan and next() scans, and check that the information is show in the JSON view of the TaskMonitor REVISION DETAIL https://reviews.facebook.net/D3045 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/6915/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        amirshim has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        Can't override Mikhail diff, so created a new diff: https://reviews.facebook.net/D3045

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        BRANCH
        add_the_table_name_and_cf_name_for_the_next_HBASE-5045_v3

        Show
        Phabricator added a comment - amirshim has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Can't override Mikhail diff, so created a new diff: https://reviews.facebook.net/D3045 REVISION DETAIL https://reviews.facebook.net/D2913 BRANCH add_the_table_name_and_cf_name_for_the_next_ HBASE-5045 _v3
        Hide
        Phabricator added a comment -

        amirshim has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        See https://reviews.facebook.net/D2913 for comments.

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        Show
        Phabricator added a comment - amirshim has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". See https://reviews.facebook.net/D2913 for comments. REVISION DETAIL https://reviews.facebook.net/D3045
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12525679/D3045.1.patch
        against trunk revision .

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

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

        +1 hadoop23. The patch compiles against the hadoop 0.23.x profile.

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

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

        +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 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.TestCheckTestClasses

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//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/12525679/D3045.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 hadoop23. The patch compiles against the hadoop 0.23.x profile. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +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 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.TestCheckTestClasses Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1772//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        Amir: does this version address comments from the other diff?

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Amir: does this version address comments from the other diff? REVISION DETAIL https://reviews.facebook.net/D3045
        Hide
        Phabricator added a comment -

        mbautin has abandoned the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        REVISION DETAIL
        https://reviews.facebook.net/D2913

        Show
        Phabricator added a comment - mbautin has abandoned the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". REVISION DETAIL https://reviews.facebook.net/D2913
        Hide
        Phabricator added a comment -

        amirshim has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        Mikhail: Most of the comments in the other diff were addressed with comments in the other diff, but I added some more comments where needed in this diff.

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        Show
        Phabricator added a comment - amirshim has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Mikhail: Most of the comments in the other diff were addressed with comments in the other diff, but I added some more comments where needed in this diff. REVISION DETAIL https://reviews.facebook.net/D3045
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        INLINE COMMENTS
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:30 Please add @Category(SmallTests.class)

        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:42 Two extra empty lines should be removed.
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:77 Please insert space between catch and (
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:131 This class and MyFailureCaseServer2 can be made private, right ?

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        Show
        Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:30 Please add @Category(SmallTests.class) src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:42 Two extra empty lines should be removed. src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:77 Please insert space between catch and ( src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:131 This class and MyFailureCaseServer2 can be made private, right ? REVISION DETAIL https://reviews.facebook.net/D3045
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        INLINE COMMENTS
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:101 I think in general we don't put more than one class in a file. Either use a static inner class or put this class in its own file.
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:131 Make a static inner class or move to a separate file.
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:147 Make a static inner class or move to a separate file.

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        Show
        Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:101 I think in general we don't put more than one class in a file. Either use a static inner class or put this class in its own file. src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:131 Make a static inner class or move to a separate file. src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java:147 Make a static inner class or move to a separate file. REVISION DETAIL https://reviews.facebook.net/D3045
        Hide
        Phabricator added a comment -

        amirshim updated the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".
        Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg

        Fixes based on comments.

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java

        To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim

        Show
        Phabricator added a comment - amirshim updated the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg Fixes based on comments. REVISION DETAIL https://reviews.facebook.net/D3045 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12526352/D3045.2.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1829//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/12526352/D3045.2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1829//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".

        lgtm.

        Please resolve the following conflict and submit to Hadoop QA again.

        Hunk #2 FAILED at 2306.
        1 out of 2 hunks FAILED – saving rejects to file src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java.rej

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:202 'avail' -> 'available'
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:195 'a RPC' -> 'an RPC'
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:197 Remove 'an ' before 'example'

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim

        Show
        Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". lgtm. Please resolve the following conflict and submit to Hadoop QA again. Hunk #2 FAILED at 2306. 1 out of 2 hunks FAILED – saving rejects to file src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java.rej INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:202 'avail' -> 'available' src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:195 'a RPC' -> 'an RPC' src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java:197 Remove 'an ' before 'example' REVISION DETAIL https://reviews.facebook.net/D3045 To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim
        Hide
        Phabricator added a comment -

        amirshim updated the revision "[jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info".
        Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg

        Rebased

        REVISION DETAIL
        https://reviews.facebook.net/D3045

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java
        src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java
        src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java
        src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java

        To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim

        Show
        Phabricator added a comment - amirshim updated the revision " [jira] HBASE-5045 Annotation for Custom Param formatting and next() RPC call info". Reviewers: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg Rebased REVISION DETAIL https://reviews.facebook.net/D3045 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandler.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTask.java src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java src/main/java/org/apache/hadoop/hbase/util/ParamFormat.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatHelper.java src/main/java/org/apache/hadoop/hbase/util/ParamFormatter.java src/test/java/org/apache/hadoop/hbase/util/TestParamFormatter.java To: mbautin, Liyin, tedyu, stack, JIRA, nspiegelberg, amirshim
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12526400/D3045.3.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1838//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/12526400/D3045.3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1838//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        The next() call is no longer in HRegionServer.java for trunk.
        See:

            public Result next() throws IOException {
        src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        
        Show
        Ted Yu added a comment - The next() call is no longer in HRegionServer.java for trunk. See: public Result next() throws IOException { src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java

          People

          • Assignee:
            Amir Shimoni
            Reporter:
            Liyin Tang
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development