[
https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13098281#comment-13098281
]
jirapos...@reviews.apache.org commented on HBASE-4014:
------------------------------------------------------
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > I think there's still one important tweak to the exception handling --
if we trigger a server abort on a coprocessor exception, we should make sure
that an exception is thrown back to the client. Otherwise there's no
indication on the client side that any problem occurred.
bq. >
bq. > Other than that, there are some style problems throughout the patch to
clean up:
bq. >
bq. > - brace alignment: should be "} else {" and "} catch (...) {" -- on same
line as closing brace
bq. > - spaces follow commas in method parameters: "method(arg1, arg2)"
bq. > - limit lines to 80 chars
bq. > - some unnecessary empty lines
Hi Gary, thanks for your comments. All style problems have been fixed in my
latest patch.
Throwing an exception back to the client if the server aborts is a good idea,
but I'm having difficulties implementing this. If the server throws an
exception, then it can't abort and if it aborts, then it's too late for it to
throw an exception back to the client. Perhaps the server could communicate to
the client that it's going to abort in some other way?
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java,
line 615
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line615>
bq. >
bq. > Combine into a single comment?
Fixed, thanks.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java,
line 626
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line626>
bq. >
bq. > Move up to prev line: } else {
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling 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. > I think the name of the config variable should be
hbase.coprocessor.abortonerror. No other hbase configuration property uses
underscores in the name.
bq. >
bq. > Also, you can just use:
bq. > if (conf.getBoolean("hbase.coprocessor.abortonerror", false)) {
bq. >
bq. > since CoprocessorHost has it's own instance-level Configuration
reference.
Changed to "hbase.coprocessor.abortonerror", also using your more concise
"conf.getBoolean()" syntax; thanks.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java,
line 636
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36017#file36017line636>
bq. >
bq. > Move up to prev line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling 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. > This should be below the else block so it's always thrown.
bq. >
bq. > In the case the region server is aborted, we actually want an
exception to come back to the client. Otherwise we're burying the error and
the client just assumes the operation succeeded, which is bad.
I think this is the same difficulty with the server not being able to both
throw an exception and abort.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 97
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line97>
bq. >
bq. > Should be on prev line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 99
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line99>
bq. >
bq. > Args should have a space after a comma
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 101
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line101>
bq. >
bq. > Should be on prev line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 107
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line107>
bq. >
bq. > Empty line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 109
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line109>
bq. >
bq. > Empty line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 123
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36019#file36019line123>
bq. >
bq. > Prev line
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. >
src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java,
line 207
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36021#file36021line207>
bq. >
bq. > Limit lines to 80 char
bq. >
bq. > hookName isn't used here, do we need it? Stack trace will point to
the method name anyway.
Fixed line length and removed unneeded "hookName" parameter.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. > src/main/resources/hbase-default.xml, line 729
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36022#file36022line729>
bq. >
bq. > As stated above, I think it should be hbase.coprocessor.abortonerror
Fixed.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 83
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36023#file36023line83>
bq. >
bq. > Should this check the type of exception that's expected?
Added exception type-checking and comments regarding the NullPointerException
(expected: test passes) versus IOException (unexpected: test fails).
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionServerCoprocessorException.java,
line 68
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36024#file36024line68>
bq. >
bq. > Does this really need to be a thread? Why not just do the put
synchronously?
Good point; removed thread and simply doing the put synchronously, thanks.
bq. On 2011-08-29 22:42:09, Gary Helmling wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionServerCoprocessorException.java,
line 85
bq. > <https://reviews.apache.org/r/969/diff/8/?file=36024#file36024line85>
bq. >
bq. > If the put triggers a RS abort, I think we should be expecting an
exception here. Wouldn't we want to make sure it's thrown to notify the client
of the error?
As above, if the server is configured to abort (as in this test), then it can't
throw an exception since if it did, it can't abort.
- Eugene
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/969/#review1667
-----------------------------------------------------------
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
