[
https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13056932#comment-13056932
]
[email protected] commented on HBASE-4014:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/969/#review927
-----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1990>
Is it better to add a toString() to environment classes? It will be useful
for other cases.
Here:
coprocessorSetAsString += env.toString();
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1991>
Append a space after the comma?
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1992>
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.
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1993>
According to coding style guideline: avoid lines longer than 80 characters.
src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1994>
I may miss something here: why they have to be treated differently? Any
problem if we throw other IOE here?
src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1995>
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.
src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
<https://reviews.apache.org/r/969/#comment1996>
white spaces.
src/test/java/org/apache/hadoop/hbase/coprocessor/BuggyCoprocessor.java
<https://reviews.apache.org/r/969/#comment1999>
Rename it to BuggyRegionObserver to avoid confusion?
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
<https://reviews.apache.org/r/969/#comment2002>
2011 instead of 2010?
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
<https://reviews.apache.org/r/969/#comment2001>
space between parameters?
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
<https://reviews.apache.org/r/969/#comment2000>
empty lines.
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
<https://reviews.apache.org/r/969/#comment1997>
Do you need to override all the method from the abstract class? I think you
only need to override postCreateTable().
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java
<https://reviews.apache.org/r/969/#comment1998>
Rename it to BuggyMasterObserver to avoid confusion?
- Mingjie
On 2011-06-27 20:04:21, 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-06-27 20:04:21)
bq.
bq.
bq. Review request for 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
0a1fb2a
bq. src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java
4800bea
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
a98117f
bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
ab16880
bq. src/test/java/org/apache/hadoop/hbase/coprocessor/BuggyCoprocessor.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.
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
>
>
> 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
