[jira] [Commented] (HBASE-17817) Make Regionservers log which tables it removed coprocessors from when aborting

["Yu Li (JIRA)" <ji...@apache.org>]

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

Yu Li commented on HBASE-17817:
-------------------------------

The idea LGTM, some comments on current patch:

{code}
if(RegionCoprocessorEnvironment.class.isAssignableFrom(env.getClass()))
{code}
Since we have the passed in object, I suggest to use {{instanceof}} instead of {{isAssignableFrom}}, say {{env instanceof RegionCoprocessorEnvironment}}

And I'd suggest to use {{String#format}} to reduce duplicated code, like:
{code}
      String tableName = null;
      if (env instanceof RegionCoprocessorEnvironment) {
        try {
          tableName =
              ((RegionCoprocessorEnvironment) env).getRegionInfo().getTable().getNameAsString();
        } catch (NullPointerException npe) {
          if (LOG.isTraceEnabled()) {
            LOG.trace("Failed to get table name from environment", npe);
          }
        }
      }
      String formatter =
          "Removing coprocessor '" + env.toString() + "' from %s because it threw:  " + e;
      String logMsg;
      if (tableName != null) {
        logMsg = String.format(formatter, "table '" + tableName + "'");
      } else {
        logMsg = String.format(formatter, "environment");
      }
      LOG.error(logMsg, e);
{code}

Regarding the added test case, it's good for manual check but not automated. So I'd suggest to remove it from the patch and simply post the message we got during manual check. This is what I saw in my execution:
{noformat}
2017-04-06 16:55:04,465 ERROR [RpcServer.default.FPBQ.Fifo.handler=4,queue=0,port=52716] coprocessor.CoprocessorHost(623): 
Removing coprocessor 'org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost$RegionEnvironment@525cb06c' from table 
'testCoprocessorTableEndpoint' because it threw:  java.lang.RuntimeException: Foo throws
java.lang.RuntimeException: Foo throws
	at org.apache.hadoop.hbase.coprocessor.TestCoprocessorHostRemoveFromTable$FailingCoprocessor.prePut(TestCoprocessorHostRemoveFromTable.java:58)
	at org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost$28.call(RegionCoprocessorHost.java:884)
	at org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost$RegionOperation.call(RegionCoprocessorHost.java:1662)
	at org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost.execOperation(RegionCoprocessorHost.java:1745)
	at org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost.execOperation(RegionCoprocessorHost.java:1701)
	at org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost.prePut(RegionCoprocessorHost.java:880)
	at org.apache.hadoop.hbase.regionserver.HRegion.doPreBatchMutateHook(HRegion.java:3102)
	at org.apache.hadoop.hbase.regionserver.HRegion.batchMutate(HRegion.java:3080)
	at org.apache.hadoop.hbase.regionserver.HRegion.batchMutate(HRegion.java:3026)
{noformat}

And notice that the attached patch no longer compiles against master branch, since {{BaseRegionObserver}} is already removed.

> Make Regionservers log which tables it removed coprocessors from when aborting
> ------------------------------------------------------------------------------
>
>                 Key: HBASE-17817
>                 URL: https://issues.apache.org/jira/browse/HBASE-17817
>             Project: HBase
>          Issue Type: Improvement
>          Components: Coprocessors, regionserver
>    Affects Versions: 1.1.2
>            Reporter: Steen Manniche
>              Labels: logging
>         Attachments: HBASE-17817.master.001.patch
>
>
> When a coprocessor throws a runtime exception (e.g. NPE), the regionserver handles this according to {{hbase.coprocessor.abortonerror}}.
> If the coprocessor was loaded on a specific table, the output in the logs give no indication as to which table the coprocessor was removed from (or which version, or jarfile is the culprit). This causes longer debugging and recovery times.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)