[ https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13081171#comment-13081171 ]
jirapos...@reviews.apache.org commented on HBASE-4014: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/969/#review1326 ----------------------------------------------------------- For the loaded list of coprocessors, I think it would be better to invert the setup you have here. Store these as a simple static variable in CoprocessorHost. This is really part of the CP framework and it belongs there more than it belongs in HMaster or HRegionServer. In addition, the MasterServices and RegionServerServices changes then become unnecessary. Using a static variable will lump everything together in the mini-cluster setups used by the tests (since master and region servers run together in the same jvm), but I don't think this is a problem. You'll also need to use a synchronized set to properly handle concurrent loading of CPs when regions open on a RS, but again I don't think this will ultimately be much overhead as the set is only accessed on CP load and server abort. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3006> I think CoprocessorHost should have a static HashSet<String> for the loaded coprocessor class names. Then add a static method to access the property: public static HashSet<String> getLoadedCoprocessors(); Then HMaster.abort() and HRegionServer.abort() just need to call CoprocessorHost.getLoadedCoprocessors() when logging. src/main/java/org/apache/hadoop/hbase/master/HMaster.java <https://reviews.apache.org/r/969/#comment3007> Move into CoprocessorHost src/main/java/org/apache/hadoop/hbase/master/MasterServices.java <https://reviews.apache.org/r/969/#comment3008> -1. This doesn't belong in MasterServices. src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java <https://reviews.apache.org/r/969/#comment3011> Duplicates same method in MasterCoprocessorHost? Move to CoprocessorHost and then you only need one implementation. src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java <https://reviews.apache.org/r/969/#comment3009> -1. This doesn't belong in RegionServerServices, it's part of the cp framework. src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java <https://reviews.apache.org/r/969/#comment3010> By moving loaded coprocessor set to CoprocessorHost, you don't need this anymore. - Gary On 2011-08-06 03:19:56, 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-08-06 03:19:56) 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 18ba6e7 bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java 8beeb68 bq. src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java aa930f5 bq. src/main/java/org/apache/hadoop/hbase/master/MasterServices.java 7d9fd9d bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 23225d7 bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java c44da73 bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java 8ffa086 bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCoprocessorHost.java 03df574 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/TestCatalogJanitor.java 78e7d62 bq. src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java ab12968 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