[
https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13065611#comment-13065611
]
[email protected] 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
