hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4070) [Coprocessors] Improve region server metrics to report loaded coprocessors to master
Date Fri, 30 Sep 2011 17:29:50 GMT

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

jiraposter@reviews.apache.org commented on HBASE-4070:
------------------------------------------------------



bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 3245
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46422#file46422line3245>
bq.  >
bq.  >     is it okay just reuse the existing serverLoad, without rebuilding the whole
thing? 
bq.  >     
bq.  >     this.serverInfo.getLoad()
bq.  >     
bq.  >     but it may not be a big issue.

sorry, I don't see a serverInfo member in HRegionServer..?


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HServerLoad.java, line 190
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46417#file46417line190>
bq.  >
bq.  >     an extra indent?

fixed; thanks.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HServerLoad.java, line 465
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46417#file46417line465>
bq.  >
bq.  >     Just a naming suggestion for walCoprocessors=>coprocessors. Although right
now WALObserver is the only RS level CP, but it'd better not to indicate here.

I changed the parameter name to "rsCoprocessors" and added a javadoc @param note; what do
you think? Could also just call it "coprocessors" as you suggested. 


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HServerLoad.java, line 505
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46417#file46417line505>
bq.  >
bq.  >     getSimpleName() only returns the class name w/o package info. I think we should
use the whole class name for the comparator.
bq.  
bq.  Andrew Purtell wrote:
bq.      Full classnames are canonical and avoid accidental conflict, but are unwieldy and
not so nice to look at. Judgement call, I'd say. I think it unlikely that for any given deployment
two coprocessors will have the same class name but be in a different package.

I would agree with Andrew and use getSimpleName().


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java, line 261
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46420#file46420line261>
bq.  >
bq.  >     The new method is a little odd. how about just getCoprocessors().

Removed HMasterInterface.java changes at Andy's recommendation: now no difference with trunk.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1514
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46421#file46421line1514>
bq.  >
bq.  >     still the method name?

changed to getCoprocessors() at Andy's recommendation.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 3244
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46422#file46422line3244>
bq.  >
bq.  >     same as HMasterInterface. can we just generateCoprocessorString() => String
getCoprocessors()?

changed; thanks.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 1652
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46418#file46418line1652>
bq.  >
bq.  >     do we really need to provide it thru HTAdmin? if we do, the returned value is
not correct in case of exception.

removed differences from trunk; you are right: we don't need it here.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java,
line 24
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46424#file46424line24>
bq.  >
bq.  >     The file TestCoprocessorEndpoint is particularly for testing cp endpoint. It's
odd to put the new test cases to this file. Maybe TestClassLoading is a better place.

Good idea; moved to a new source file: TestCoprocessorReporting.java.


bq.  On 2011-09-29 22:42:26, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorEndpoint.java,
line 201
bq.  > <https://reviews.apache.org/r/2029/diff/2/?file=46424#file46424line201>
bq.  >
bq.  >     according the previous override comparator in HRL, the cps should be in alphabet
order. so you don't need to compare this 2 strings.

Thanks; removed extra string and added comment with your point.


- Eugene


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2029/#review2183
-----------------------------------------------------------


On 2011-09-30 04:58:20, Eugene Koontz wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/2029/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-09-30 04:58:20)
bq.  
bq.  
bq.  Review request for hbase and Mingjie Lai.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Proposed fix for HBASE-4070. 
bq.  
bq.  
bq.  This addresses bug HBASE-4070.
bq.      https://issues.apache.org/jira/browse/HBASE-4070
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/jamon/org/apache/hbase/tmpl/master/MasterStatusTmpl.jamon abeb850 
bq.    src/main/jamon/org/apache/hbase/tmpl/regionserver/RSStatusTmpl.jamon be6fceb 
bq.    src/main/java/org/apache/hadoop/hbase/ClusterStatus.java 01bc1dd 
bq.    src/main/java/org/apache/hadoop/hbase/HServerLoad.java 0c680e4 
bq.    src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java dbae4fd 
bq.    src/main/java/org/apache/hadoop/hbase/master/HMaster.java f80d232 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 96b763b 
bq.    src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorReporting.java PRE-CREATION

bq.  
bq.  Diff: https://reviews.apache.org/r/2029/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Two new tests : testRegionServerCoprocessorReported() and testMasterServerCoprocessorsReported()
included in a new source file src/test/java/o.a.h.h/coprocessor/TestCoprocessorReporting.java.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Eugene
bq.  
bq.


                
> [Coprocessors] Improve region server metrics to report loaded coprocessors to master
> ------------------------------------------------------------------------------------
>
>                 Key: HBASE-4070
>                 URL: https://issues.apache.org/jira/browse/HBASE-4070
>             Project: HBase
>          Issue Type: Improvement
>    Affects Versions: 0.90.3
>            Reporter: Mingjie Lai
>            Assignee: Eugene Koontz
>         Attachments: HBASE-4070.patch, HBASE-4070.patch, master-web-ui.jpg, rs-status-web-ui.jpg
>
>
> HBASE-3512 is about listing loaded cp classes at shell. To make it more generic, we need
a way to report this piece of information from region to master (or just at region server
level). So later on, we can display the loaded class names at shell as well as web console.


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message