-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
An optimization about MessageFuture #831
Conversation
merge for name change
Codecov Report
@@ Coverage Diff @@
## develop #831 +/- ##
=============================================
+ Coverage 38.04% 38.05% +0.01%
+ Complexity 1006 1005 -1
=============================================
Files 220 220
Lines 8449 8452 +3
Branches 1016 1015 -1
=============================================
+ Hits 3214 3216 +2
- Misses 4850 4852 +2
+ Partials 385 384 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments
@@ -26,13 +29,12 @@ | |||
* @author jimin.jm @alibaba-inc.com | |||
* @date 2018 /10/9 | |||
*/ | |||
public class MessageFuture { | |||
public class MessageFuture{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ white space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
result = origin.get(timeout, unit); | ||
} catch (ExecutionException e) { | ||
throw new ShouldNeverHappenException("Should not get results in a multi-threaded environment", e); | ||
} catch (TimeoutException e){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ white space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@CoffeeLatte007 do u have any other questions ? |
Use CompletableFuture instead CountDownLatch in MessageFuture
Ⅰ. Describe what this PR did
In the MessageFuture class, it is currently implemented using CountDownLatch.
This synchronization method is relatively heavy, we should replace it with CompletableFuture to achieve.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews