[ https://issues.apache.org/jira/browse/HBASE-4014?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13092372#comment-13092372 ]
jirapos...@reviews.apache.org commented on HBASE-4014: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/969/#review1673 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3809> I would write: new HashSet<String>() src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3810> Should be abort(s) src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3811> Javadoc is very nice. Please reformat the paragraph so that they 're within 80 characters. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3812> Is there need to use two javadoc blocks ? src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3815> Line too long. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3817> Currently there is no config parameter with _ in its name in hbase-default.xml I think a better name would be hbase.coprocessor.abort.onerror or hbase.coprocessor.onerror.abort Or, we can name this parameter hbase.coprocessor.policy.onerror whose values can be abort or removecoproc Just throw out some suggestions. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3814> This and the above else is not necessary - exception (handling) would form natural barrier. This is a matter of style, so it is minor. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3816> We should document the behavior w.r.t. fatal error in release notes. src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java <https://reviews.apache.org/r/969/#comment3813> Line too long. src/main/java/org/apache/hadoop/hbase/master/HMaster.java <https://reviews.apache.org/r/969/#comment3818> The call below seems to be a static method call. cpHost is assigned in finishInitialization(). If your intention is to return empty string before cpHost is initialized in finishInitialization(), please say so in javadoc. src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java <https://reviews.apache.org/r/969/#comment3819> A better name might be handleCoprocessorUnknownRegion src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java <https://reviews.apache.org/r/969/#comment3820> Long lines in this area. src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java <https://reviews.apache.org/r/969/#comment3821> I think the period after Ignoring is not needed. - Ted On 2011-08-26 20:52:06, 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-26 20:52:06) 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 7a3ac1d bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java 92d5dbb bq. src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java aa930f5 bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 58f0350 bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 53645ce 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