[Dubbo-2187] Use CompletetableFuture to rewrite the embedded ListenableFuture #2213
Conversation
| } | ||
|
|
||
| public void start() { | ||
| if (!started) { | ||
| Thread connectThread = new Thread(listenableFutureTask); | ||
| Thread connectThread = new Thread(); |
There was a problem hiding this comment.
I think Thread connectThread = new Thread(); is no longer needed here, because we already started ZkClient in an async thread by invoking CompletableFuture.supplyAsync
There was a problem hiding this comment.
Besides, there are UT failures.
Failed tests:
CompletableFutureTaskTest.testCustomExecutor:90
Wanted but not invoked:
executor.execute(<any java.lang.Runnable>);
-> at org.apache.dubbo.common.concurrent.CompletableFutureTaskTest.testCustomExecutor(CompletableFutureTaskTest.java:90)
Actually, there were zero interactions with this mock.
| return newMonitor; | ||
|
|
||
| },executor); | ||
| completableFuture.thenRunAsync(new MonitorListener(key)); |
There was a problem hiding this comment.
We already defined an executor, specify this executor for thenRunAsync may be better.
| @@ -75,11 +71,11 @@ public void start() { | |||
| } | |||
|
|
|||
| public void addListener(final IZkStateListener listener) { | |||
| listenableFutureTask.addListener(new Runnable() { | |||
| completableFuture.thenRunAsync(new Runnable() { | |||
There was a problem hiding this comment.
whenComplete or handle maybe better, and try to use lambda.
| } | ||
| completableFuture.whenComplete((v,e)->{ | ||
| client = v; | ||
| client.subscribeStateChanges(listener); |
There was a problem hiding this comment.
I think we should handle the possible Exceptions that may come up here.
|
I can‘t find the reasons for failed，but test unit is success in local |
|
@dengwanghua src/main/java/org/apache/dubbo/monitor/support/AbstractMonitorFactory.java:[31] (imports) AvoidStarImport: Using the '.' form of import should be avoided - java.util.concurrent.. |
public void addListener(final IZkStateListener listener) {
completableFuture.whenComplete((v,e)->{
client = v;
client.subscribeStateChanges(listener);
});
}I think we should consider possible Exceptions that may come up here. |
…ure/1_2187 # Conflicts: # dubbo-monitor/dubbo-monitor-api/src/main/java/org/apache/dubbo/monitor/support/AbstractMonitorFactory.java # dubbo-remoting/dubbo-remoting-zookeeper/src/main/java/org/apache/dubbo/remoting/zookeeper/zkclient/ZkClientWrapper.java
What is the purpose of the change
Solve Issue #2187, Use CompletetableFuture to rewrite the embedded ListenableFuture
Brief changelog
Verifying this change
org.apache.dubbo.common.concurrent.CompletableFutureTaskTesttest passedFollow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests&mvn clean test-compile failsafe:integration-testto make sure unit-test and integration-test pass.