hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4014) Coprocessors: Flag the presence of coprocessors in logged exceptions
Date Mon, 01 Aug 2011 22:08:54 GMT

    [ https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13073821#comment-13073821
] 

jiraposter@reviews.apache.org 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

        

Mime
View raw message