HBase
  1. HBase
  2. HBASE-5827

[Coprocessors] Observer notifications on exceptions

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Coprocessors
    • Labels:
      None

      Description

      Benjamin Busjaeger wrote on dev@:

      Is there a reason that RegionObservers are not notified when a get/put/delete fails? Suppose I maintain some (transient) state in my Coprocessor that is created during preGet and discarded during postGet. If the get fails, postGet is not invoked, so I cannot remove the state.

      If there is a good reason, is there any other way to achieve the same thing? If not, would it be possible to add something the snippet below to the code base?

          // pre-get CP hook
          if (withCoprocessor && (coprocessorHost != null)) {
            if (coprocessorHost.preGet(get, results)) {
              return results;
            }
          }
      +    try{
          ...
      +    } catch (Throwable t) {
      +        // failed-get CP hook
      +        if (withCoprocessor && (coprocessorHost != null)) {
      +          coprocessorHost.failedGet(get, results);
      +        }
      +        rethrow t;
      +    }
      
          // post-get CP hook
          if (withCoprocessor && (coprocessorHost != null)) {
            coprocessorHost.postGet(get, results);
          }
      

        Activity

        Hide
        ramkrishna.s.vasudevan added a comment -

        +1 on such a feature.

        Show
        ramkrishna.s.vasudevan added a comment - +1 on such a feature.
        Hide
        stack added a comment -

        This seems like something we need if cps are to be able to keep a running context.

        Show
        stack added a comment - This seems like something we need if cps are to be able to keep a running context.
        Hide
        Andrew Purtell added a comment -

        I can try wrapping the code between pre and post hooks with try/catch in a way that doesn't change indenting inbetween.

        Show
        Andrew Purtell added a comment - I can try wrapping the code between pre and post hooks with try/catch in a way that doesn't change indenting inbetween.
        Hide
        Benjamin Busjaeger added a comment -

        It may make sense to also pass the Throwable to the Coprocessor in the 'failedXXX' methods to give it a chance to introspect the error.

        Show
        Benjamin Busjaeger added a comment - It may make sense to also pass the Throwable to the Coprocessor in the 'failedXXX' methods to give it a chance to introspect the error.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Andy
        I have some thing which is similar to this and sorry if am diverting the current JIRA title.
        Suppose i want some state variable or some common object to be used across one flow, for eg. take the case of PUT.
        May be i have the current PUT with me and from which i form the new PUTs to be added thro coprocessor hooks to a new region.
        Assume the PUT(and related things) that i created in preBatchPut is also needed while i use the preWALWrite, is there any way i can carry it in one flow?
        Hope am not missing anything? If you feel this can be done may be we can file an Improvement JIRA. Please provide your suggestions

        Show
        ramkrishna.s.vasudevan added a comment - @Andy I have some thing which is similar to this and sorry if am diverting the current JIRA title. Suppose i want some state variable or some common object to be used across one flow, for eg. take the case of PUT. May be i have the current PUT with me and from which i form the new PUTs to be added thro coprocessor hooks to a new region. Assume the PUT(and related things) that i created in preBatchPut is also needed while i use the preWALWrite, is there any way i can carry it in one flow? Hope am not missing anything? If you feel this can be done may be we can file an Improvement JIRA. Please provide your suggestions
        Hide
        Andrew Purtell added a comment -

        Assume the PUT(and related things) that i created in preBatchPut is also needed while i use the preWALWrite, is there any way i can carry it in one flow?

        You can package the WAL observer and the region observer in the same JAR and they can use any way you'd like to pass state between, perhaps by way of a singleton. Does this work?

        Show
        Andrew Purtell added a comment - Assume the PUT(and related things) that i created in preBatchPut is also needed while i use the preWALWrite, is there any way i can carry it in one flow? You can package the WAL observer and the region observer in the same JAR and they can use any way you'd like to pass state between, perhaps by way of a singleton. Does this work?
        Hide
        Andrew Purtell added a comment - - edited

        I'd like to revisit the API changes under consideration here.

        Do we want a new failXXX for every operation, or should we alter the postXXX methods?

        If the latter, then do we:

        provide backwards compatible postXXX methods that put the Throwable into the Context and provide a new Context method for getting the caught Throwable, or null if nothing was caught;

        or pass the caught Throwable to new postXXX methods as a new parameter, still provide backwards compatible postXXX methods that put the Throwable into the Context, but also deprecate the backwards compatible postXXX methods.

        Edit: To clarify a bit, with the alternatives above involving postXXX, we would move code between preXXX and postXXX hooks into a try block, and in the catch handler call postXXX. The choice presented is basically between introducing a new failXXX method for every hook point or extending postXXX semantics to cover the exception path.

        Show
        Andrew Purtell added a comment - - edited I'd like to revisit the API changes under consideration here. Do we want a new failXXX for every operation, or should we alter the postXXX methods? If the latter, then do we: provide backwards compatible postXXX methods that put the Throwable into the Context and provide a new Context method for getting the caught Throwable, or null if nothing was caught; or pass the caught Throwable to new postXXX methods as a new parameter, still provide backwards compatible postXXX methods that put the Throwable into the Context, but also deprecate the backwards compatible postXXX methods. Edit: To clarify a bit, with the alternatives above involving postXXX, we would move code between preXXX and postXXX hooks into a try block, and in the catch handler call postXXX. The choice presented is basically between introducing a new failXXX method for every hook point or extending postXXX semantics to cover the exception path.
        Hide
        Jesse Yates added a comment -

        provide backwards compatible postXXX methods that put the Throwable into the Context and provide a new Context method for getting the caught Throwable, or null if nothing was caught;

        This seems a bit wonky and could will probably break a lot of existing CP functionality (or at least be incredibly wasteful, generating the same puts again, etc).

        pass the caught Throwable to new postXXX methods as a new parameter,

        This seems a good way to do the failXXX method and not make things too different.

        We should also consider another interface based on (or at least conceptually similar to) SU's async stuff, so you could register a callback if you are interested in the error, but otherwise is handled by the standard mechanisms. Probably going to be a bit cleaner that doing all this wrapping and calling stuff on failure. Maybe a scope expansion here, but seemed a reasonable application.

        Show
        Jesse Yates added a comment - provide backwards compatible postXXX methods that put the Throwable into the Context and provide a new Context method for getting the caught Throwable, or null if nothing was caught; This seems a bit wonky and could will probably break a lot of existing CP functionality (or at least be incredibly wasteful, generating the same puts again, etc). pass the caught Throwable to new postXXX methods as a new parameter, This seems a good way to do the failXXX method and not make things too different. We should also consider another interface based on (or at least conceptually similar to) SU's async stuff, so you could register a callback if you are interested in the error, but otherwise is handled by the standard mechanisms. Probably going to be a bit cleaner that doing all this wrapping and calling stuff on failure. Maybe a scope expansion here, but seemed a reasonable application.
        Hide
        Andrew Purtell added a comment -

        We should also consider another interface based on (or at least conceptually similar to) SU's async stuff, so you could register a callback if you are interested in the error

        @Jesse, thanks for commenting. I agree.

        For illustration, what the above discussion so far considers is something like:

            if (withCoprocessor && (coprocessorHost != null)) {
              if (coprocessorHost.preGet(context, get, results)) {
                return results;
              }
            }
        +   try {
        
        // Existing code with formatting unchanged
        
        +    } catch (Throwable t) {
        +        // failed-get CP hook
        +        if (withCoprocessor && (coprocessorHost != null)) {
        +          coprocessorHost.postGet(context, t, get, results);
        +        }
        +        rethrow t;
        +    }
            if (withCoprocessor && (coprocessorHost != null)) {
        -     coprocessorHost.postGet(context, get, results);
        +     coprocessorHost.postGet(context, null, get, results);
            }
        

        What would the alternative look like?

        Show
        Andrew Purtell added a comment - We should also consider another interface based on (or at least conceptually similar to) SU's async stuff, so you could register a callback if you are interested in the error @Jesse, thanks for commenting. I agree. For illustration, what the above discussion so far considers is something like: if (withCoprocessor && (coprocessorHost != null)) { if (coprocessorHost.preGet(context, get, results)) { return results; } } + try { // Existing code with formatting unchanged + } catch (Throwable t) { + // failed-get CP hook + if (withCoprocessor && (coprocessorHost != null)) { + coprocessorHost.postGet(context, t, get, results); + } + rethrow t; + } if (withCoprocessor && (coprocessorHost != null)) { - coprocessorHost.postGet(context, get, results); + coprocessorHost.postGet(context, null, get, results); } What would the alternative look like?
        Hide
        Andrew Purtell added a comment -

        This seems a bit wonky and could will probably break a lot of existing CP functionality (or at least be incredibly wasteful, generating the same puts again, etc).

        Maybe I don't follow.

        Consider again:

        +    } catch (Throwable t) {
        +        // failed-get CP hook
        +        if (withCoprocessor && (coprocessorHost != null)) {
        +          coprocessorHost.postGet(context, t, get, results);
        +        }
        +        rethrow t;
        +    }
        

        and then in psuedocode:

        postGet(context, t, get, results):
            context.setThrowable(t)
            postGet(context, get, results)
        

        So actually this would not break any existing CPs at all, it's the backwards compatible option.

        Show
        Andrew Purtell added a comment - This seems a bit wonky and could will probably break a lot of existing CP functionality (or at least be incredibly wasteful, generating the same puts again, etc). Maybe I don't follow. Consider again: + } catch (Throwable t) { + // failed-get CP hook + if (withCoprocessor && (coprocessorHost != null)) { + coprocessorHost.postGet(context, t, get, results); + } + rethrow t; + } and then in psuedocode: postGet(context, t, get, results): context.setThrowable(t) postGet(context, get, results) So actually this would not break any existing CPs at all, it's the backwards compatible option.
        Hide
        Jesse Yates added a comment -

        So actually this would not break any existing CPs at all, it's the backwards compatible option.

        Yeah, you're right. I mis-understood and thought it would be more along the lines of calling cp.put(...) where the context there also includes the exception (since you probably care about the original put that borked things). This is better.

        Show
        Jesse Yates added a comment - So actually this would not break any existing CPs at all, it's the backwards compatible option. Yeah, you're right. I mis-understood and thought it would be more along the lines of calling cp.put(...) where the context there also includes the exception (since you probably care about the original put that borked things). This is better.
        Hide
        Jesse Yates added a comment -

        The alternative might we something like:

        Set<ErrorHandler> CPerrorHandlers = …
        …
        loadCPs(){
        for(cp : loadedCPs){
          CPerrorHandlers.add( cp.getErrorHandler());
        }
        …
        

        Conceptually, the success/failure handlers could be bound via a Callback<success, failure> on the actual operation. The success handler would just be the regular Observer (though maybe it might be worth looking into abstracting that into another class that the Observer would return on request to be added to the Callback). The failure handler would deal with the exception and be preloaded with the rest of the context.

        Conceptually, this is really close to how the CP host works already, so maybe I'm just proposing just doing a class-name change - you've definitely thought about this interface more that me Ideally, I would like to see the operation just wrapped in a single handler that keeps track of the success/failure observers, and calls them as necessary (without dealing with too much other state, e.g. host != null feels clunky all over the place).

        Show
        Jesse Yates added a comment - The alternative might we something like: Set<ErrorHandler> CPerrorHandlers = … … loadCPs(){ for(cp : loadedCPs){ CPerrorHandlers.add( cp.getErrorHandler()); } … Conceptually, the success/failure handlers could be bound via a Callback<success, failure> on the actual operation. The success handler would just be the regular Observer (though maybe it might be worth looking into abstracting that into another class that the Observer would return on request to be added to the Callback). The failure handler would deal with the exception and be preloaded with the rest of the context. Conceptually, this is really close to how the CP host works already, so maybe I'm just proposing just doing a class-name change - you've definitely thought about this interface more that me Ideally, I would like to see the operation just wrapped in a single handler that keeps track of the success/failure observers, and calls them as necessary (without dealing with too much other state, e.g. host != null feels clunky all over the place).
        Hide
        Andrew Purtell added a comment -

        The postXXX hook is already what you are calling a success handler.

        We would still need to get into position to catch the failure. Invoking the failure handler from a catch clause would be like failXXX but with another name (and class), right?

        Show
        Andrew Purtell added a comment - The postXXX hook is already what you are calling a success handler. We would still need to get into position to catch the failure. Invoking the failure handler from a catch clause would be like failXXX but with another name (and class), right?
        Hide
        Andrew Purtell added a comment -

        I don't want to hijack this discussion but when you say:

        Conceptually, the success/failure handlers could be bound via a Callback<success, failure> on the actual operation.

        this brings me back to an early thought I had about Coprocessors: that there would be a number of onThis and onThat style callbacks and the callback code (lambdas) would be "submitted" in client side APIs, and shipped to the server to execute. No need to install server side extensions at all. But for a variety of reasons we went instead with a server side upcall model, and that's what we have now.

        Show
        Andrew Purtell added a comment - I don't want to hijack this discussion but when you say: Conceptually, the success/failure handlers could be bound via a Callback<success, failure> on the actual operation. this brings me back to an early thought I had about Coprocessors: that there would be a number of onThis and onThat style callbacks and the callback code (lambdas) would be "submitted" in client side APIs, and shipped to the server to execute. No need to install server side extensions at all. But for a variety of reasons we went instead with a server side upcall model, and that's what we have now.
        Hide
        Jesse Yates added a comment -

        the postXXX hook is already what you are calling a success handler.

        Yup, just trying to formalize the concept a little bit.

        nvoking the failure handler from a catch clause would be like failXXX but with another name (and class), right?

        Yeah, but I'd rather have that encapsulated away from the main code, so the calling methods would just be able to call

        ...
        internalPutter.put(stuff)
        ...
        

        But again, basically what you have already (clearly more thought through than I did in the last 30mins). I'll have to take some time to see if a bigger redesign is warranted.

        TL;DR +1 on an onXXXXFailure method + a deprecated postXXX with error in context for compatibility

        Show
        Jesse Yates added a comment - the postXXX hook is already what you are calling a success handler. Yup, just trying to formalize the concept a little bit. nvoking the failure handler from a catch clause would be like failXXX but with another name (and class), right? Yeah, but I'd rather have that encapsulated away from the main code, so the calling methods would just be able to call ... internalPutter.put(stuff) ... But again, basically what you have already (clearly more thought through than I did in the last 30mins). I'll have to take some time to see if a bigger redesign is warranted. TL;DR +1 on an onXXXXFailure method + a deprecated postXXX with error in context for compatibility
        Hide
        Andrew Purtell added a comment - - edited

        I'll have to take some time to see if a bigger redesign is warranted.

        In the abstract I'd be all for a "coprocessors v2" if the result is more elegant. We are still pre Singularity. (But that does imply a time horizon.) The current framework design is the familiar upcall model, akin to kernel extensions, or a VFS interface. It counts simplicity and familiarity as pluses. But it is by no means the last word I'd say.

        TL;DR +1 on an onXXXXFailure method + a deprecated postXXX with error in context for compatibility

        Ack. And thanks.

        Show
        Andrew Purtell added a comment - - edited I'll have to take some time to see if a bigger redesign is warranted. In the abstract I'd be all for a "coprocessors v2" if the result is more elegant. We are still pre Singularity. (But that does imply a time horizon.) The current framework design is the familiar upcall model, akin to kernel extensions, or a VFS interface. It counts simplicity and familiarity as pluses. But it is by no means the last word I'd say. TL;DR +1 on an onXXXXFailure method + a deprecated postXXX with error in context for compatibility Ack. And thanks.
        Hide
        Andrew Purtell added a comment -

        @Jesse, for the sake of clarity I'm reading your +1 as a +1 on: "a new postXXX method that accepts an additional Throwable parameter plus a deprecated postXXX with that Throwable in the context for compatibility", as that is what was proposed. Please indicate if otherwise.

        Show
        Andrew Purtell added a comment - @Jesse, for the sake of clarity I'm reading your +1 as a +1 on: "a new postXXX method that accepts an additional Throwable parameter plus a deprecated postXXX with that Throwable in the context for compatibility", as that is what was proposed. Please indicate if otherwise.
        Hide
        Jesse Yates added a comment -

        @Andrew - yup, that sounds fine. Are you thinking reflection to check existence of the right method or just calling both? Maybe some sort of tiered thing where the default calls the original?

        Show
        Jesse Yates added a comment - @Andrew - yup, that sounds fine. Are you thinking reflection to check existence of the right method or just calling both? Maybe some sort of tiered thing where the default calls the original?
        Hide
        Andrew Purtell added a comment - - edited

        Maybe some sort of tiered thing where the default calls the original

        Yes.

        So if the CP overrides this, it's done there.

        If not, the base Observer class that CPs are supposed to be extending will call the original.

        Edit: And the aim is source level compatibility only.

        Show
        Andrew Purtell added a comment - - edited Maybe some sort of tiered thing where the default calls the original Yes. So if the CP overrides this, it's done there. If not, the base Observer class that CPs are supposed to be extending will call the original. Edit: And the aim is source level compatibility only.
        Hide
        Jonathan Hsieh added a comment -

        Minor clarification – what happens if the postXXX method throws an exception? What if you want the postXXX method to eat the exception (because it somehow handles it)?

        Specifically instead of this:

        // Existing code with formatting unchanged
        
        +    } catch (Throwable t) {
        +        // failed-get CP hook
        +        if (withCoprocessor && (coprocessorHost != null)) {
        +          coprocessorHost.postGet(context, t, get, results);
        +        }
        +        rethrow t;
        +    }
            if (withCoprocessor && (coprocessorHost != null)) {
        -     coprocessorHost.postGet(context, get, results);
        +     coprocessorHost.postGet(context, null, get, results);
            }
        

        would it make more sense to do this?

        // Existing code with formatting unchanged
        
        +    } catch (Throwable t) {
        +        // failed-get CP hook
        +        if (withCoprocessor && (coprocessorHost != null)) {
        +          coprocessorHost.postGet(context, t, get, results);  // coproc has option to rethrow which would percolate out, or eat the exn if it handled it.
        +          return; 
        +        }
        +        rethrow t; // no coproc so just rethrow
        +    }
            if (withCoprocessor && (coprocessorHost != null)) {
        -     coprocessorHost.postGet(context, get, results);
        +     coprocessorHost.postGet(context, null, get, results);  // normal success case; coproc can throw exn as well.
            }
        

        I'd also suggest that the base observer class implementation would check if there was a non-null exn argument and rethrow; if there was no exn it would do nothing (or maybe just call the original version with no exn argument).

        Show
        Jonathan Hsieh added a comment - Minor clarification – what happens if the postXXX method throws an exception? What if you want the postXXX method to eat the exception (because it somehow handles it)? Specifically instead of this: // Existing code with formatting unchanged + } catch (Throwable t) { + // failed-get CP hook + if (withCoprocessor && (coprocessorHost != null )) { + coprocessorHost.postGet(context, t, get, results); + } + rethrow t; + } if (withCoprocessor && (coprocessorHost != null )) { - coprocessorHost.postGet(context, get, results); + coprocessorHost.postGet(context, null , get, results); } would it make more sense to do this? // Existing code with formatting unchanged + } catch (Throwable t) { + // failed-get CP hook + if (withCoprocessor && (coprocessorHost != null )) { + coprocessorHost.postGet(context, t, get, results); // coproc has option to rethrow which would percolate out, or eat the exn if it handled it. + return ; + } + rethrow t; // no coproc so just rethrow + } if (withCoprocessor && (coprocessorHost != null )) { - coprocessorHost.postGet(context, get, results); + coprocessorHost.postGet(context, null , get, results); // normal success case ; coproc can throw exn as well. } I'd also suggest that the base observer class implementation would check if there was a non-null exn argument and rethrow; if there was no exn it would do nothing (or maybe just call the original version with no exn argument).
        Hide
        Andrew Purtell added a comment -

        @Jon, both suggestions good, +1 and +1.

        Show
        Andrew Purtell added a comment - @Jon, both suggestions good, +1 and +1.
        Hide
        Andrew Purtell added a comment -

        I'm going to let this issue stew for a few days and circle back next week to implement.

        Show
        Andrew Purtell added a comment - I'm going to let this issue stew for a few days and circle back next week to implement.

          People

          • Assignee:
            Andrew Purtell
            Reporter:
            Andrew Purtell
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:

              Development