Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async optimization #3738

Merged
merged 41 commits into from
Apr 12, 2019
Merged

Async optimization #3738

merged 41 commits into from
Apr 12, 2019

Conversation

chickenlj
Copy link
Contributor

What is the purpose of the change

Fixes #3287

Brief changelog

GenericService provides extra CompletableFuture method.
Invoker chain
Embedded Filter implementations.

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

chickenlj and others added 27 commits March 1, 2019 15:07
use constant to replace magic number
# Conflicts:
#	dubbo-common/src/main/java/org/apache/dubbo/common/Constants.java
#	dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/Invoker.java
#	dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java
#	dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java
#	dubbo-remoting/dubbo-remoting-api/src/main/java/org/apache/dubbo/remoting/exchange/support/DefaultFuture.java
#	dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/filter/ActiveLimitFilter.java
#	dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/filter/FutureFilter.java
#	dubbo-rpc/dubbo-rpc-rest/src/test/java/org/apache/dubbo/rpc/protocol/rest/RestProtocolTest.java
#	dubbo-rpc/dubbo-rpc-rest/src/test/java/org/apache/dubbo/rpc/protol/rest/RestProtocolTest.java
# Conflicts:
#	dubbo-common/src/test/java/org/apache/dubbo/common/config/PropertiesConfigurationTest.java
#	dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java
#	dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/filter/FutureFilter.java
#	dubbo-rpc/dubbo-rpc-rest/src/test/java/org/apache/dubbo/rpc/protocol/rest/RestProtocolTest.java
}

default void onError(Throwable t, Invoker<?> invoker, Invocation invocation) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking of grouping onResponse onError into a Listener interface, they are all callbacks and should not being defined directly in Filter.

@ralf0131
Copy link
Contributor

ralf0131 commented Apr 5, 2019

This pull request is too large to review... Could you provide a guide to the reviewers?

@ralf0131
Copy link
Contributor

ralf0131 commented Apr 5, 2019

All the UT are failing. Could you take a look at it?

@chickenlj
Copy link
Contributor Author

This pull request is too large to review... Could you provide a guide to the reviewers?

Sorry, but I am afraid this one has to be this way. I tried to refactor the whole call process inside Dubbo to make it totally asynchronous, the changes in this pull request are all restricted to this goal and they are almost the minimum changesets to make the framework work again.

I will try to provide a reviewing guide to help anyone who's willing to review.

@chickenlj
Copy link
Contributor Author

#3847

@chickenlj
Copy link
Contributor Author

chickenlj commented Apr 10, 2019

Although many files have been modified in the PR, there are only 3 key points, all other changes are made to adapt to these core designs.

  1. Refactoring of internal Filters, making them work with the async pattern by registering Listener to each Filter.
@SPI
public interface Filter {
    /**
     * Does not need to override/implement this method.
     */
    Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException;

    interface Listener {

        void onResponse(Result result, Invoker<?> invoker, Invocation invocation);

        void onError(Throwable t, Invoker<?> invoker, Invocation invocation);
    }

}
  1. Use AsyncRpcResult as the type return back to the Filter chain.
  • Add status interface to Result interface
public interface Result extends Serializable {
  
    Result thenApplyWithContext(Function<Result, Result> fn);

    <U> CompletableFuture<U> thenApply(Function<Result, ? extends U> fn);

    Result get() throws InterruptedException, ExecutionException;

}
  • AsyncRpcResult
public class AsyncRpcResult implements Result {
    private CompletableFuture<AppResponse> responseFuture;
    ...
}
  • Rename RpcResult to AppResponse with the meaning no changed
public class AppResponse implements Result, Serializable {
}
  1. Add async method with CompletableFuture signature to GenericService
public interface GenericService {

    /**
     * Generic invocation
     *
     * @param method         Method name, e.g. findPerson. If there are overridden methods, parameter info is
     *                       required, e.g. findPerson(java.lang.String)
     * @param parameterTypes Parameter types
     * @param args           Arguments
     * @return invocation return value
     * @throws GenericException potential exception thrown from the invocation
     */
    Object $invoke(String method, String[] parameterTypes, Object[] args) throws GenericException;

    default CompletableFuture<Object> $invokeAsync(String method, String[] parameterTypes, Object[] args) throws GenericException {
        Object object = $invoke(method, parameterTypes, args);
        if (object instanceof CompletableFuture) {
            return (CompletableFuture<Object>) object;
        }
        return CompletableFuture.completedFuture(object);
    }

}

Below are the async and sync workflows respectively:
3 0 async workflow

3 0 async-to-sync workflow

@chickenlj chickenlj requested a review from Jeff-Lv April 11, 2019 01:17
Copy link
Contributor

@Jeff-Lv Jeff-Lv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jeff-Lv Jeff-Lv merged commit 003e400 into apache:3.x-dev Apr 12, 2019
chickenlj added a commit to chickenlj/incubator-dubbo that referenced this pull request May 8, 2019
* Result implement CF

* Result implement CF

* Result implement CF

* Add AsyncRpcResult

* Fix bugs and refactor Filter

* Try to add onSend onError for Filter

* invoke different filter method according to result status.

*  make generic work with async call, including add $invokeAsync

* refactor legacy Filter implementation to work with onResponse.

* demo changes

* Fixes apache#3620, provider attachment lose on consumer side, fix this by reverting RpcContext copy

* AsyncRpcResult should always holds an Invocation instance

* refactor filter signature

* reimplement embedded Filters

* use ProviderModel modification in 3.x

* Fix address notification processing workflow after merging 3.x branch

* Fix UT

* Fix UT

* Unit test of JValidator; Clean code of JValidator (apache#3723)

* Fixes apache#3625 (apache#3730)

use constant to replace magic number

* Fix conflict when merging master and 3.x

* Fix conflict when merging master and 3.x

* Result interface itself has Future status.

* Fix DefaultFuture UT

* Wrap all protocol Invoker with AsyncToSyncInvoker & Fix UT

* Add license

* fix UT

* Fix ut in MonitorFilterTest

* avoid duplicate async to sync wrapper

* return async result in CacheFilter.

* fix UT in CacheFilterTest

* Add generic condition check to GenericFilter callback.

* Fix UT

* Get generic from RpcContext if the value in Invocation is empty.

* Fix RSocketProtocol to meet AbstractProtocol adjustment

* rename RpcResult to AppResponse to help avoid confusion with AsyncRpcResult.

* RSocket module switch to AsyncRpcResult
@chickenlj chickenlj mentioned this pull request May 8, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants