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

remove @Async from core framework. #3095

Merged
merged 4 commits into from
Jan 2, 2019
Merged

Conversation

chickenlj
Copy link
Contributor

@chickenlj chickenlj commented Dec 29, 2018

What is the purpose of the change

move @AsyncFor from core framework to side project.

Brief changelog

remove @AsyncFor, call async counterpart directly.

Verifying this change

see dubbo/dubbo-samples

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.

@codecov-io
Copy link

codecov-io commented Dec 29, 2018

Codecov Report

Merging #3095 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #3095      +/-   ##
===========================================
- Coverage     63.52%   63.5%   -0.02%     
  Complexity       75      75              
===========================================
  Files           653     653              
  Lines         28208   28196      -12     
  Branches       4791    4790       -1     
===========================================
- Hits          17918   17906      -12     
- Misses         8025    8027       +2     
+ Partials       2265    2263       -2
Impacted Files Coverage Δ Complexity Δ
...in/java/org/apache/dubbo/rpc/support/RpcUtils.java 53.6% <ø> (+0.07%) 0 <0> (ø) ⬇️
...g/apache/dubbo/rpc/proxy/AbstractProxyInvoker.java 50% <0%> (ø) 0 <0> (ø) ⬇️
.../apache/dubbo/rpc/protocol/dubbo/DubboInvoker.java 73.33% <100%> (+1.66%) 0 <0> (ø) ⬇️
...ache/dubbo/rpc/proxy/InvokerInvocationHandler.java 50% <100%> (+16.66%) 0 <0> (ø) ⬇️
.../apache/dubbo/qos/protocol/QosProtocolWrapper.java 64.1% <0%> (-17.95%) 0% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyServer.java 67.85% <0%> (-3.58%) 0% <0%> (ø)
...pache/dubbo/remoting/transport/AbstractServer.java 44.79% <0%> (-3.13%) 0% <0%> (ø)
...dubbo/remoting/exchange/support/DefaultFuture.java 71.81% <0%> (-1.35%) 0% <0%> (ø)
...pache/dubbo/registry/support/AbstractRegistry.java 81.2% <0%> (-0.76%) 0% <0%> (ø)
.../apache/dubbo/remoting/transport/AbstractPeer.java 63.04% <0%> (+4.34%) 0% <0%> (ø) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bc1312...93a72ac. Read the comment docs.

@khanimteyaz
Copy link
Contributor

@chickenlj would be possible to explain for mine understanding purpose moving @Asynch from core to side project?

public static boolean hasGeneratedFuture(Method method) {
return method.isAnnotationPresent(AsyncFor.class) && hasFutureReturnType(method);
}

public static boolean isFutureReturnType(Invocation inv) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an thought should we consider this name to ** isReturnTypeFuture** instead of isFutureReturnType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@chickenlj
Copy link
Contributor Author

chickenlj commented Dec 29, 2018

@khanimteyaz Thanks for your attention, I will try to explain.

As you may have noticed, we support a service definition with a CompletableFuture as the return type.

public interface AsyncService {
    CompletableFuture<String> sayHello(String name);
}

But this is newly introduced in 2.7.0, which means, there would be many legacy services that can not leverage this feature. Because if they change the service definition (the interface definition) they will have to reimplement their service to meet the new definition.
For example, here is a typical service definition before 2.7.0

public interface GreetingsService {
    String sayHi(String name);
}

To be able to call this service asynchronously, they have to use RpcContext to get the Future, like this:

// the invoke will return null immediately
greetingsService.sayHi("world");
// get the Future instance, the future will get notified once the result returns.
Future<String> future = RpcContext.getContext().getFuture();

Annotation @AsyncFor is a mechanism I introduced to simplify the process for those legacy services, you can refer to here for how it works.

But now, as long as I realized we have supported java 8, we can use the default method to avoid generating a new class. Here is the new way I have come up with, this is what mean by saying `move the @AsyncFor from core to side project '. This way, this would become a convenient solution we provide but not a mandatory solution provided in core.

@khanimteyaz
Copy link
Contributor

@khanimteyaz Thanks for your attention, I will try to explain.

As you may have noticed, we support a service definition with a CompletableFuture as the return type.

public interface AsyncService {
    CompletableFuture<String> sayHello(String name);
}

But this is newly introduced in 2.7.0, which means, there would be many legacy services that can not leverage this feature. Because if they change the service definition (the interface definition) they will have to reimplement their service to meet the new definition.
For example, here is a typical service definition before 2.7.0

public interface GreetingsService {
    String sayHi(String name);
}

To be able to call this service asynchronously, they have to use RpcContext to get the Future, like this:

// the invoke will return null immediately
greetingsService.sayHi("world");
// get the Future instance, the future will get notified once the result returns.
Future<String> future = RpcContext.getContext().getFuture();

Annotation @AsyncFor is a mechanism I introduced to simplify the process for those legacy services, you can refer to here for how it works.

But now, as long as I realized we have supported java 8, we can use the default method to avoid generating a new class. Here is the new way I have come up with, this is what mean by saying `move the @AsyncFor from core to side project '. This way, this would become a convenient solution we provide but not a mandatory solution provided in core.

@chickenlj Thanks a lot for the details. Appreciate it.

I will give a look into this PR.

@khanimteyaz
Copy link
Contributor

This looks to fine ok to me.

Thinking loud here (please correct me if I am wrong here). As here we are enabling dubbo consumer to make async call to service method, but to achieve this, service provider has to define the stub with CompletableFuture. Here whether a consumer can make a async call depends on whether service developer has enabled service interface with CompletableFuture or not in some form.

The decision of communication mode (async or async) is consumer choice. So thinking, how can we make our provider and consumer independent or each other in this context.

e.g. here an an analogy I am using to provide the approach
java hashmap is not synchronized but java collections come up with Collection.jave and provided a utility method java.util.Collections.synchronizedMap() which actually wrap hashmap with thread safety behavior.

@chickenlj Do you is there something we can do now or in future?

@chickenlj
Copy link
Contributor Author

The decision of communication mode (async or async) is consumer choice. So thinking, how can we make our provider and consumer independent or each other in this context.

This is a good point @khanimteyaz, I think I got what you want to express. Maybe we can also provide a utility method or sth like that. I am on a new year's vacation now, I will think about it as soon as I come back to work.

@chickenlj
Copy link
Contributor Author

By the way, the implementation of the async feature has been an enhncent or patch effort based on the legacy code. I think it definitely deserves an refactor.
For example, you can find that we have three types of Result, they areAsyncRpcResult, RpcResult, SimpleAsyncRpcResult.

  • RpcResult are for sync call
  • AsyncRpcResult and SimpleAsyncRpcResult are for different types of async calls
  • AsyncRpcResult inside composes two futures.
    But I think we should unify them to one RpcResult, which should be an instance of CompletationStage.

Given that hou have put forward many constructive suggestions recently, which are of great help to us. It would be great if you've got some experience or you'd like to take a look at this part.

@chickenlj chickenlj merged commit 04aae2a into apache:master Jan 2, 2019
@chickenlj
Copy link
Contributor Author

I will merge this PR to prepare for the current release.

@khanimteyaz
Copy link
Contributor

By the way, the implementation of the async feature has been an enhncent or patch effort based on the legacy code. I think it definitely deserves an refactor.
For example, you can find that we have three types of Result, they areAsyncRpcResult, RpcResult, SimpleAsyncRpcResult.

  • RpcResult are for sync call
  • AsyncRpcResult and SimpleAsyncRpcResult are for different types of async calls
  • AsyncRpcResult inside composes two futures.
    But I think we should unify them to one RpcResult, which should be an instance of CompletationStage.

Given that hou have put forward many constructive suggestions recently, which are of great help to us. It would be great if you've got some experience or you'd like to take a look at this part.

@chickenlj Thanks for your detail information. I will try my best to suggest something which if there is something good I can comeup. Thanks for your appreciation.
For the same I have created an issue 3113

@chickenlj
Copy link
Contributor Author

For the same I have created an issue #3113

Looking forward to it.

khanimteyaz pushed a commit to khanimteyaz/incubator-dubbo that referenced this pull request Jan 18, 2019
* remove @async from core framework.

* refactor method name to isReturnFuture
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.

3 participants