Operating System: Linux
Platform: PC

Tony - to add comments / link this bugs to another one where we are unifying the exceptions etc.

The other bug talks about redesigning the error handling so we should take these points into consideration to make it easier to use.

I pushed https://git.opendaylight.org/gerrit/#/c/8915/ to improve the javadocs on RpcResult and RpcError and also added a DSL RpcResultBuilder that should make it easier to construct instances.

Re: the error handling and code samples in the desc, it seems there's 2 issues here. One is about the commit method where errors can be reported in 2 different ways: via an unchecked ExecutionException and via the RpcResult. I agree it would be ideal if there was one mechanism. In fact, that is up for discussion next week on a call.

The other issue is similar but concerns returning errors from RPC implementations. Re: this sample code:

public Future<RpcResult<Void>> exampleRpc(ExampleRpcInput input)
throws Exception

I see a couple issues with this approach. First, throwing just Exception is not a good practice - it's too general (see http://stackoverflow.com/questions/7966148/throws-exception-bad-practice. Also Sonar tools report this as a violation). Throwing some unchecked exception would be even worse - callers would have no idea any ex could be thrown.

Second, by throwing an exception, we're back to the first issue except there would 3 mechanisms to report errors to the caller: 1) via the RPC call itself 2) via ExecutionException wrapping an ex set in the Future 3) the RpcResult. We wouldn't want to do #1 anyway because an RPC impl may be performed async/non-blocking so any error information needs to be reported via the returned Future. The above code does a blocking commit call which isn't best practice.

I think we want a clearly defined way in the API to return structured error information. RpcError, which is patterned after netconf error reporting, in RpcResult provides this mechanism.

RpcError info could be wrapped by a well-known exception type (similar to RestconfDpcumentedException). However, returning an exception via the Future results in it being wrapped in an ExecutionException to the caller. Callers would have to know to look for the well-known exception (only way would be via javadoc which can be missed) and would need to check for instanceof on the ExecutionException cause. Thus doesn't seem clean to me.

So, given the async nature of RPCs, I think reporting error info via the RpcResult is the most effective way.

I wasn't saying we should literally use that exact signature. Note I also said we could get rid of RpcResult, which should have been a clue . Having a return value plus the ability to throw an exception gives you 100% of what you need.

The RpcError type should be eliminated entirely. You can create a RpcException object with derived exceptions for each of the type fields in RpcError. The other fields are, from what I can see, not useful, but if you want you can include those as well but expect most applications not to bother with them. Most of those things are only related to the transport layer anyway which the application wouldn't ever care about. If you want to make the method signature clean, you can make this an unchecked exception and document its existence with an @throws javadoc comment in the generated code. The marshaling code needs to handle unchecked exceptions in any event.

While we're at it, get rid of the "Input" object and define the method with parameters for each of the fields in the Input object instead. The Output object should only be created if there's more than one value being returned. This will help to eliminate further boilerplate.

Sorry I missed your clue.

It sounds like your saying RPC methods should return a result and throw an RpcException, ie

public ExampleResult exampleRpc(ExampleRpcInput input) throws RpcException

, or am I missing another clue ? Assuming not, that would be fine, except, how do you handle returning a result or error async? With this signature, you can't - you're method has to do blocking calls and block the caller.

In order to do async, the RPC method must return a Future. The only way to return an RpcException is via wrapping in an ExecutionException that is thrown by the Future. We could do that but I don't think that's as clean and intuitive as explicitly returning RpcErrors in the RpcResult. Not to mention the headache of deprecating the currently generated method signatures and migrating to a new signature.

(In reply to Rob Adams from comment #3)
> I wasn't saying we should literally use that exact signature. Note I also
> said we could get rid of RpcResult, which should have been a clue .
> Having a return value plus the ability to throw an exception gives you 100%
> of what you need.
>
> The RpcError type should be eliminated entirely. You can create a
> RpcException object with derived exceptions for each of the type fields in
> RpcError. The other fields are, from what I can see, not useful, but if you
> want you can include those as well but expect most applications not to
> bother with them. Most of those things are only related to the transport
> layer anyway which the application wouldn't ever care about. If you want to
> make the method signature clean, you can make this an unchecked exception
> and document its existence with an @throws javadoc comment in the generated
> code. The marshaling code needs to handle unchecked exceptions in any event.
>
> While we're at it, get rid of the "Input" object and define the method with
> parameters for each of the fields in the Input object instead. The Output
> object should only be created if there's more than one value being returned.
> This will help to eliminate further boilerplate.

Another way, instead of returning a Future, pass a callback to the RPC method, something like:

public interface RpcCallback<T> {
void onSuccess( T result );
void onFailure( Collection<RpcError> errors );
// OR void onFailure( RpcException error );
}

public void exampleRpc(ExampleRpcInput input, RpcCallback<ExampleResult> callback );

(In reply to Tom Pantelis from comment #4)
> Sorry I missed your clue.
>
> It sounds like your saying RPC methods should return a result and throw an
> RpcException, ie
>
> public ExampleResult exampleRpc(ExampleRpcInput input) throws RpcException
>
> , or am I missing another clue ? Assuming not, that would be fine,
> except, how do you handle returning a result or error async? With this
> signature, you can't - you're method has to do blocking calls and block the
> caller.
>
> In order to do async, the RPC method must return a Future. The only way to
> return an RpcException is via wrapping in an ExecutionException that is
> thrown by the Future. We could do that but I don't think that's as clean and
> intuitive as explicitly returning RpcErrors in the RpcResult. Not to mention
> the headache of deprecating the currently generated method signatures and
> migrating to a new signature.
>
>
> (In reply to Rob Adams from comment #3)
> > I wasn't saying we should literally use that exact signature. Note I also
> > said we could get rid of RpcResult, which should have been a clue .
> > Having a return value plus the ability to throw an exception gives you 100%
> > of what you need.
> >
> > The RpcError type should be eliminated entirely. You can create a
> > RpcException object with derived exceptions for each of the type fields in
> > RpcError. The other fields are, from what I can see, not useful, but if you
> > want you can include those as well but expect most applications not to
> > bother with them. Most of those things are only related to the transport
> > layer anyway which the application wouldn't ever care about. If you want to
> > make the method signature clean, you can make this an unchecked exception
> > and document its existence with an @throws javadoc comment in the generated
> > code. The marshaling code needs to handle unchecked exceptions in any event.
> >
> > While we're at it, get rid of the "Input" object and define the method with
> > parameters for each of the fields in the Input object instead. The Output
> > object should only be created if there's more than one value being returned.
> > This will help to eliminate further boilerplate.

The Future return can be potentially useful, though not always.

Let's say I have the following:

rpc test-rpc {
input {
leaf input1

}
}

The method signature could be:

ListenableFuture<String> testRPC(String input1, Integer input2)
throws RpcException {
if (somethingwentwrong)
throw new RpcApplicationException("The horror!");
}

If this RPC is going to do something quick and return an immediatefuture, it might be simpler to allow them to specify in the model something like ext:synchronous true;

which would make the signature be:
String testRPC(String input1, Integer input2)
throws RpcException {

}
in this case the framework could wrap it with whatever fake async semantics needed. Probably a large majority of these "RPC" functions would be something quick and synchronous and in-process.

Removing the Future is not as large an improvement however so I don't think we should bother with it right away, and it could be added later.

To make the transition possible, I would recommend that in the yang generator maven plugin we add a parameter that will enable the new RPC generation style that would be off by default. We should print a warning that says eventually the default will change. At the same time, we can also fix MDSAL-18.

Another option would be:

ListenableFuture<String> testRPC(String input1, Integer input2) {
if (somethingwentwrong)
return Futures.failedFuture(new RpcApplicationException("The horror!"));
}

This is probably better, though we could wrap the call with something that would catch an exception and return a failed future.

Yeah you'd want to always throw the RpcException via Future. If RpcException was declared in the method signature then callers would have to handle it in 2 places:

The RpcException would get wrapped in an ExecutionException. However, using Futures.addCallback makes this easier on the caller side:

ListenableFuture<String> future = testRPC( ... );
Futures.addCallback( future, new FutureCallback<String>() {
@Override
public void onSuccess( String result ) {
}

@Override
public void onFailure( Throwable ex ) {

if( ex instanceof RpcException ) {
}
}
} );

Now that I think about it some more and see code written, it looks reasonable. As long as we can effectively convey in javadocs that callers can expect only RpcException as a failure. The MD-SAL RPC proxy framework could also ensure only RpcExceptions propagate to the caller by intercepting any other unchecked exception from the RPC impl via a proxy ListenableFuture and converting to RpcException. Also, if an unchecked exception was thrown directly from the method by the impl, the proxy framework could catch it, convert to RpcException and wrap in an immediateFailedFuture. Maybe it already does something similar - not sure.

Re: changing the method param signature, that's probably another discussion/bug enhancement. I can see that being tricky for rest conf to call RPCs generically. Right now it uses generated codecs to invoke the *InputBuilder generically with the input data.

(In reply to Rob Adams from comment #7)
> Another option would be:
>
> ListenableFuture<String> testRPC(String input1, Integer input2)

>
> This is probably better, though we could wrap the call with something that
> would catch an exception and return a failed future.

Invoking a method with arguments is just as easy as invoking a method with one argument.

It would be ideal to return a CheckFuture:

CheckedFuture<String,RpcException> testRPC( ... );

A potential use case with the current the usage of RpcResult is that you could return a successful RpcResult and also report warnings/errors to the caller. An example of this is in the toaster example where the KitchenService reports success if the toaster is out of bread with a warning RpcError as it was able to make the rest of the breakfast but just missing the toast. I suppose this could be conveyed via an RpcException but it may not be as easy and/or intuitive.

This type of use case wouldn't be common though in real scenarios.

I'd say that even if you did want to return some status information that could include a partial failure, you'd want to model that in the output object of your RPC and not use the RpcError thing anyway.

Move to MDSAL

I think we really want a wrapper service, which will take care of the mapping and exception handling.

At the end of the day, we really want to have a simple service which can translate a Future<RpcErrorAware> to a Future<RpcResult<?>>. Should probably be a static method somewhere.

Note the Future.get() part can be handled as a transformation.

So, with MDSAL-777 this gets much easier. Each constituent RPC has a org.opendaylight.yangtools.yang.binding.Rpc specialization, which has the same invocation method name and the generated interface acts only as an identifier.
Introduce something like:

interface RpcProviderService {

    <I extends RpcInput, O extends RpcOutput> @NonNull Registration registerRpcImplementation(Class<? extends Rpc<I, O>> rpcType, SynchronousRpc<I, O> implementation);

    @FuctionalInterface
    interface SynchronousRpc<I extends RpcInput, O extends RpcOutput> {
        // null implies no RpcResult.getResult()
        @Nullable O invokeSimple(@NonNull I input) throws RpcException;
    }

    final class RpcException extends Exception {

        // checked to be non-empty
        @NonNull List<RpcError> rpcErrors();
    }
}

Users then can do:

import FooRpc;

RpcProviderService svc;
var reg = svc.registerRpcImplementation(FooRpc.class, input -> {
    return new FooOutputBuilder()
        // whatever
        .build();
    // or throw RpcException(new RpcErrorBuilder...build())
    // or 'return null' for empty
    });

Once we have this done, we can then deal with defining an asynchronous counterpart.