[jira] [Comment Edited] (HBASE-11733) Avoid copy-paste in Master/Region CoprocessorHost

["Andrew Purtell (JIRA)" <ji...@apache.org>]

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

Andrew Purtell edited comment on HBASE-11733 at 8/15/14 5:55 PM:
-----------------------------------------------------------------

bq. We already allocate 1 object, which is the ObserverContext. if the CoprocessorOperation extends the ObserverContext, you have 1 allocations that does both.

So in the v2 patch execOperation looks like:

{code}
   private boolean execOperation(final CoprocessorOperation ctx) throws IOException {
     boolean bypass = false;
     for (RegionServerEnvironment env: coprocessors) {
       if (env.getInstance() instanceof RegionServerObserver) {
        ctx.prepare(env);
         Thread currentThread = Thread.currentThread();
         ClassLoader cl = currentThread.getContextClassLoader();
         try {
           currentThread.setContextClassLoader(env.getClassLoader());
           ctx.call((RegionServerObserver)env.getInstance(), ctx);
         } catch (Throwable e) {
           handleCoprocessorThrowable(env, e);
         } finally {
           currentThread.setContextClassLoader(cl);
         }
         bypass |= ctx.shouldBypass();
         if (ctx.shouldComplete()) {
           break;
         }
       }
       ctx.postEnvCall(env);
     }
     return bypass;
   }
{code}

IMO, this is better. It doesn't look bad, and we will save a bit on young collection workload

bq. if we want to push also for the coprocessor list may be empty and in the old code we avoided the allocation we can also do: coprocessors.isEmpty() ? null : new CoprocessorOperation()

Also sounds like a good idea.


was (Author: apurtell):
bq. We already allocate 1 object, which is the ObserverContext. if the CoprocessorOperation extends the ObserverContext, you have 1 allocations that does both.

So in the v2 patch execOperation looks like:

{code}
   private boolean execOperation(final CoprocessorOperation ctx) throws IOException {
     boolean bypass = false;
     for (RegionServerEnvironment env: coprocessors) {
       if (env.getInstance() instanceof RegionServerObserver) {
        ctx.prepare(env);
         Thread currentThread = Thread.currentThread();
         ClassLoader cl = currentThread.getContextClassLoader();
         try {
           currentThread.setContextClassLoader(env.getClassLoader());
           ctx.call((RegionServerObserver)env.getInstance(), ctx);
         } catch (Throwable e) {
           handleCoprocessorThrowable(env, e);
         } finally {
           currentThread.setContextClassLoader(cl);
         }
         bypass |= ctx.shouldBypass();
         if (ctx.shouldComplete()) {
           break;
         }
       }
       ctx.postEnvCall(env);
     }
     return bypass;
   }
{code}

IMO, this is better. It doesn't look bad, and we will save a bit on young collection workload

bq. if we want to push also for the coprocessor list may be empty and in the old code we avoided the allocation we can also do: coprocessors.isEmpty() ? null : new CoprocessorOperation()

Also sounds look a good idea.

> Avoid copy-paste in Master/Region CoprocessorHost
> -------------------------------------------------
>
>                 Key: HBASE-11733
>                 URL: https://issues.apache.org/jira/browse/HBASE-11733
>             Project: HBase
>          Issue Type: Bug
>          Components: Coprocessors
>            Reporter: Matteo Bertozzi
>            Assignee: Matteo Bertozzi
>            Priority: Trivial
>             Fix For: 2.0.0
>
>         Attachments: HBASE-11733-0.98-v1.patch, HBASE-11733-v0.patch, HBASE-11733-v2.patch
>
>
> all the methods of MasterCoprocessorHost and RegionCoprocessorHost are copy-paste, and a couple of them have some missing fixes.
> The body of each function is the same, the only difference is the call to the specific coprocessor.



--
This message was sent by Atlassian JIRA
(v6.2#6252)