[
https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13073821#comment-13073821
]
[email protected] commented on HBASE-4014:
------------------------------------------------------
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > Some comments below Eugene. This thing looks useful and almost done.
Lets get it in!
Thank you Michael for looking at this!
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java,
line 54
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25643#file25643line54>
bq. >
bq. > Do we need to add this? Doesn't every object inherit Object and so
have a toString?
Thanks, Michael; you are right. Removed this unnecessary declaration.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java,
line 578
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25644#file25644line578>
bq. >
bq. > Do you think this needed Eugene? Is coprocessors a List? What if
you toString'd it? Maybe'll do right thing (with square bracket delimiters
rather than curly's but that might be ok)
Hi Michael, coprocessors is an o.a.h.h.util.SortedCopyOnWriteSet, whose
toString() returns :
"org.apache.hadoop.hbase.util.SortedCopyOnWriteSet@4d441b16" (or a similar
memory location after the @-sign.
Thus the need for a string serialization method like
"coprocessorSetAsString()".
Perhaps this should be moved to an (overriding) SortedCopyOnWriteSet's
toString()? I am happy to make a sub-JIRA to HBASE-4014 if you think so.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 81
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25645#file25645line81>
bq. >
bq. > Whats masterServices? I think it subclasses Server? If you do
getServerName or something, that'll give you something better than 'master'.
It'll include port and startcode which could be important debugging (more
important for the RS case than for Master but could be important if
multimasters).
Correct; masterServices is an instance of MasterServices, which subclasses
Server.
I think you're right that the port and startcode are important.
But note that abortServiceWithCoprocessorInfo() *does* show
serverName.getServerName() in its output, (so it shows the port and startcode
as you recomend).
On the other hand, getServerName() doesn't show what *role* the server plays:
is it a master or a regionserver? It seems to me this should be part of the
abort message, too.
So here's an example of an entire abort message on the master side:
Aborting service: master running on : 192.168.0.136,56238,1312228544878 because
coprocessor:
org.apache.hadoop.hbase.coprocessor.TestMasterCoprocessorException$BuggyMasterObserver@1658fe12
threw an exception: java.lang.NullPointerException. Loaded coprocessors are:
{class
org.apache.hadoop.hbase.coprocessor.TestMasterCoprocessorException$BuggyMasterObserver}
How does that look to you?
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 87
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25645#file25645line87>
bq. >
bq. > Nice comment
Thank you! :)
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 112
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25645#file25645line112>
bq. >
bq. > Abort seems radical
Hmm..is it? The point of HBASE-4014 is to catch buggy coprocessors by aborting
the coprocessor host (the master or regionserver) with as much information as
possible so that the bug in the coprocessor can be fixed. For example,
BuggyMasterObserver (in TestMasterCoprocessorException.java) tries to
dereference a null pointer.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java,
line 237
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25645#file25645line237>
bq. >
bq. > The convention is to put } catch { on the one line rather than line
break after the } (no biggie)
Thank you; fixed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java,
line 26
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25647#file25647line26>
bq. >
bq. > We are importing but we don't seem to use the imports, is that so?
You are right: removed unused "import o.a.h.h.coprocessor.Coprocessor;" as well
as "import o.a.h.h.coprocessor.CoprocessorHost;".
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterCoprocessorException.java,
line 65
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25651#file25651line65>
bq. >
bq. > No need for these extra new lines
Thank you for catching that; removed extra new lines.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java, line
36
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25653#file25653line36>
bq. >
bq. > Unused import?
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java, line
279
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25653#file25653line279>
bq. >
bq. > Added line we don't need?
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java,
line 49
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25654#file25654line49>
bq. >
bq. > Unused import
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/master/TestClockSkewDetection.java, line
34
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25655#file25655line34>
bq. >
bq. > Unused import?
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/master/TestLogsCleaner.java, line
37
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25656#file25656line37>
bq. >
bq. > Unused import?
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java,
line 39
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25657#file25657line39>
bq. >
bq. > Unused import?
Thanks; removed. I should have caught all these before review; not sure how
they crept in. Will try to be more careful.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java,
line 219
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25657#file25657line219>
bq. >
bq. > Addition of commented out code
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java,
line 43
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25658#file25658line43>
bq. >
bq. > Unused import
Thanks; removed.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java,
line 353
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25648#file25648line353>
bq. >
bq. > Is there any diff in the above changes?
These are a couple of typos in ZooKeeperWatcher.java that I noticed
incidentally - I will file a separate JIRA for these and remove from patch.
bq. On 2011-07-27 05:14:19, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java, line 227
bq. > <https://reviews.apache.org/r/969/diff/2/?file=25649#file25649line227>
bq. >
bq. > Same here
Another incidental typo - will note & fix in the above separate JIRA and remove
from this patch.
- Eugene
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/969/#review1198
-----------------------------------------------------------
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
