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 Thu, 14 Jul 2011 23:40:05 GMT

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

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



bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  >

Thanks for all your comments, Mingjie. Please see new patch for HBASE-4014 here: https://issues.apache.org/jira/secure/attachment/12486509/HBASE-4014.patch


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 547
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21686#file21686line547>
bq.  >
bq.  >     Is it better to add a toString() to environment classes? It will be useful for
other cases. 
bq.  >     
bq.  >     Here:
bq.  >     coprocessorSetAsString += env.toString();

Thanks Mingjie, added in next version of patch.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 548
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21686#file21686line548>
bq.  >
bq.  >     Append a space after the comma?

Thanks Mingjie, added in next version of patch.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 551
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21686#file21686line551>
bq.  >
bq.  >     how about using a flag in the above loop to determine whether to insert a comma
or not? so you don't need the indexof()+substring() here.

Thanks Mingjie, added a counter to compare with the set's size to accomplish this.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java, line 559
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21686#file21686line559>
bq.  >
bq.  >     According to coding style guideline: avoid lines longer than 80 characters.

Thanks Mingjie; fixed.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java, line 91
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21687#file21687line91>
bq.  >
bq.  >     I may miss something here: why they have to be treated differently? Any problem
if we throw other IOE here?

Hi Mingjie,
Many coprocessor hooks (e.g. preMove() and postMove()) throw UnknownRegionException (rather
than the more general IOException) to communicate errors to the client because they were written
to match the exception type thrown by their corresponding main action (e.g. HMasterInterface.move()
throws UnknownRegionException, therefore preMove() and postMove() throw this also.).

I had to write handleCoprocessorThrowableAsUnknownRegionException() for these hooks. It's
arguable that coprocessors should all be declared to throw IOException, in which case, we
would not need handleCoprocessorThrowableAsUnknownRegionException().


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java, line
193
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21688#file21688line193>
bq.  >
bq.  >     Like above, why there are 2 different handlers here. From the perspective of
cp application developers, they may expect to see all exceptions handled the same for all
hooks.

In this case, some Regionserver coprocessor hooks, for example, postClose(), are declared
to *NOT* throw exceptions: handleCoprocessorThrowableNoRethrow() handles those cases. Non-IOException
throwables should be treated as fatal coprocessor bugs and should cause the regionserver to
abort.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java, line
1174
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21688#file21688line1174>
bq.  >
bq.  >     white spaces.

Sorry, will have to fix that later when I have a better git tool that shows whitespace better.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/BuggyCoprocessor.java, line 27
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21690#file21690line27>
bq.  >
bq.  >     Rename it to BuggyRegionObserver to avoid confusion?

Fixed, thanks.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 2
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21691#file21691line2>
bq.  >
bq.  >     2011 instead of 2010?

Fixed, thanks; also fixed the same in TestRegionServerCoprocessorException.java.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 57
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21691#file21691line57>
bq.  >
bq.  >     space between parameters?

Fixed, thanks.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 67
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21691#file21691line67>
bq.  >
bq.  >     empty lines.

Fixed, thanks.


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 93
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21691#file21691line93>
bq.  >
bq.  >     Do you need to override all the method from the abstract class? I think you
only need to override postCreateTable().

Thanks, removed unneeded overrides. Still need postStartMaster() and start() (in addition
to postCreateTable()).


bq.  On 2011-06-29 01:14:09, Mingjie Lai wrote:
bq.  > src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 94
bq.  > <https://reviews.apache.org/r/969/diff/1/?file=21691#file21691line94>
bq.  >
bq.  >     Rename it to BuggyMasterObserver to avoid confusion?

Thanks, renamed according to your suggestion.


- Eugene


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


On 2011-07-14 23:39:07, 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-07-14 23:39:07)
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/master/MasterCoprocessorHost.java aa930f5 
bq.    src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 54ccd6f

bq.    src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 18ba6e7 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java c2b3558

bq.    src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 8ffa086

bq.    src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java 4fa82c0 
bq.    src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 6af0ecf 
bq.    src/test/java/org/apache/hadoop/hbase/coprocessor/BuggyRegionObserver.java PRE-CREATION

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.    src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java 7f19c72 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 78e7d62 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestClockSkewDetection.java c571227 
bq.    src/test/java/org/apache/hadoop/hbase/master/TestLogsCleaner.java fc05e47 
bq.    src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java
aa48c22 
bq.    src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
dd5dc3e 
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
>
>
> 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