hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4014) Coprocessors: Flag the presence of coprocessors in logged exceptions
Date Tue, 06 Sep 2011 19:11:17 GMT

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

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



bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 74
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line74>
bq.  >
bq.  >     I would write:
bq.  >     new HashSet<String>()

Fixed, thanks.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 222
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line222>
bq.  >
bq.  >     Should be abort(s)

Changed to "if server (master or regionserver) aborts."


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 607
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line607>
bq.  >
bq.  >     Javadoc is very nice.
bq.  >     Please reformat the paragraph so that they
bq.  >     're within 80 characters.

Fixed.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 614
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line614>
bq.  >
bq.  >     Is there need to use two javadoc blocks ?

Consolidated to one block.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 631
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line631>
bq.  >
bq.  >     Line too long.

Fixed.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 633
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line633>
bq.  >
bq.  >     Currently there is no config parameter with _ in its name in hbase-default.xml
bq.  >     
bq.  >     I think a better name would be hbase.coprocessor.abort.onerror or hbase.coprocessor.onerror.abort
bq.  >     
bq.  >     Or, we can name this parameter hbase.coprocessor.policy.onerror whose values
can be abort or removecoproc
bq.  >     
bq.  >     Just throw out some suggestions.

Changed to "hbase.coprocessor.abortonerror" per Gary's suggestion.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 637
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line637>
bq.  >
bq.  >     This and the above else is not necessary - exception (handling) would form natural
barrier. This is a matter of style, so it is minor.

Removed extraneous else {..} here.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 640
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line640>
bq.  >
bq.  >     Line too long.

Fixed, thanks.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 638
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line638>
bq.  >
bq.  >     We should document the behavior w.r.t. fatal error in release notes.

I'd be happy to help with the release notes: something like:

"A loaded coprocessor which throws a Throwable that is not an IOException will be removed
from the active coprocessor set. A LOG.error() will be written in the the affected service's
log file describing which coprocessor threw the error and what the error was that caused the
coprocessor to be removed. This is the default behavior, but if the configuration property
'hbase.coprocessor.abortonerror' is set to true, then the service hosting the erroneous coprocessor
will be aborted, which may be useful in testing and development environments."


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 1200
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36018#file36018line1200>
bq.  >
bq.  >     The call below seems to be a static method call. cpHost is assigned in finishInitialization().
bq.  >     If your intention is to return empty string before cpHost is initialized in
finishInitialization(), please say so in javadoc.

Added comment : "The set of loaded coprocessors is stored in a static set. Since it's
statically allocated, it does not require that HMaster's cpHost be
initialized prior to accessing it." Also made getLoadedCoprocessors() static since it simply
calls a static method itself. 


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java, line 91
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line91>
bq.  >
bq.  >     A better name might be handleCoprocessorUnknownRegion

Changed to handleCoprocessorUnknownRegion, thanks.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java, line 103
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line103>
bq.  >
bq.  >     Long lines in this area.

Fixed.


bq.  On 2011-08-27 19:52:59, Ted Yu wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java, line 106
bq.  > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line106>
bq.  >
bq.  >     I think the period after Ignoring is not needed.

Fixed.


- Eugene


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


On 2011-09-06 19:08:59, Eugene Koontz wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/969/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-09-06 19:08:59)
bq.  
bq.  
bq.  Review request for hbase, Gary Helmling and Mingjie Lai.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  https://issues.apache.org/jira/browse/HBASE-4014 Coprocessors: Flag the presence of coprocessors
in logged exceptions
bq.  
bq.  The general gist here is to wrap each of {Master,RegionServer}CoprocessorHost's coprocessor
call inside a 
bq.  
bq.  "try { ... } catch (Throwable e) { handleCoprocessorThrowable(e) }"
bq.  
bq.  block. 
bq.  
bq.  handleCoprocessorThrowable() is responsible for either passing 'e' along to the client
(if 'e' is an IOException) or, otherwise, aborting the service (Regionserver or Master).
bq.  
bq.  The abort message contains a list of the loaded coprocessors for crash analysis.
bq.  
bq.  
bq.  This addresses bug HBASE-4014.
bq.      https://issues.apache.org/jira/browse/HBASE-4014
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 4e492e1 
bq.    src/main/java/org/apache/hadoop/hbase/master/HMaster.java 3f60653 
bq.    src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java aa930f5 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8ff6e62 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 5796413

bq.    src/main/resources/hbase-default.xml 2c8f44b 
bq.    src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
PRE-CREATION 
bq.    src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionServerCoprocessorException.java
PRE-CREATION 
bq.  
bq.  Diff: https://reviews.apache.org/r/969/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  patch includes two tests:
bq.  
bq.  TestMasterCoprocessorException.java
bq.  TestRegionServerCoprocessorException.java
bq.  
bq.  both tests pass in my build environment.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Eugene
bq.  
bq.



> Coprocessors: Flag the presence of coprocessors in logged exceptions
> --------------------------------------------------------------------
>
>                 Key: HBASE-4014
>                 URL: https://issues.apache.org/jira/browse/HBASE-4014
>             Project: HBase
>          Issue Type: Improvement
>          Components: coprocessors
>            Reporter: Andrew Purtell
>            Assignee: Eugene Koontz
>             Fix For: 0.92.0
>
>         Attachments: HBASE-4014.patch, HBASE-4014.patch, HBASE-4014.patch, HBASE-4014.patch,
HBASE-4014.patch
>
>
> For some initial triage of bug reports for core versus for deployments with loaded coprocessors,
we need something like the Linux kernel's taint flag, and list of linked in modules that show
up in the output of every OOPS, to appear above or below exceptions that appear in the logs.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message