You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org> on 2011/07/15 01:40:05 UTC
[jira] [Commented] (HBASE-4014) Coprocessors: Flag the presence of
coprocessors in logged exceptions
[ 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