-
Notifications
You must be signed in to change notification settings - Fork 910
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
Separate RequestContextAwareFuture version overrides into mrjar… #2127
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2127 +/- ##
============================================
+ Coverage 73.65% 73.76% +0.11%
+ Complexity 9518 9498 -20
============================================
Files 835 834 -1
Lines 36691 36586 -105
Branches 4528 4526 -2
============================================
- Hits 27024 26987 -37
+ Misses 7332 7269 -63
+ Partials 2335 2330 -5
Continue to review full report at Codecov.
|
.../src/main/java9/com/linecorp/armeria/internal/Java9RequestContextAwareCompletableFuture.java
Show resolved
Hide resolved
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, @anuraaga!
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.
👍
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! @anuraaga
I left some nits :-)
core/src/main/java9/com/linecorp/armeria/internal/Java9RequestContextAwareMinimalStage.java
Outdated
Show resolved
Hide resolved
core/src/main/java9/com/linecorp/armeria/internal/Java9RequestContextAwareMinimalStage.java
Outdated
Show resolved
Hide resolved
core/src/main/java9/com/linecorp/armeria/internal/Java9RequestContextAwareMinimalStage.java
Outdated
Show resolved
Hide resolved
core/src/main/java9/com/linecorp/armeria/internal/Java9RequestContextAwareMinimalStage.java
Outdated
Show resolved
Hide resolved
core/src/main/java9/com/linecorp/armeria/internal/Java9VersionSpecific.java
Outdated
Show resolved
Hide resolved
…e#2127) … source sets. Now that we are using mrjar for version-specific implementations, we can use it for the recent change for completablefuture to support Java 9, and future ones to support Java 12. Advantages - Can use other Java 9 APIs when implementing a Java 9 API (e.g., `super.completeAsync`) - Maybe some small performance by not needing e.g., `maybeMakeContextAware`, `supplyAsync` for completion of current future - Possible readability improvement by being able to use `@Override` and otherwise IDE-awareness of the API level Disadvantage - If user shades Armeria into non-mrjar and runs on newer Java, newer APIs wouldn't function correctly
…e#2127) … source sets. Now that we are using mrjar for version-specific implementations, we can use it for the recent change for completablefuture to support Java 9, and future ones to support Java 12. Advantages - Can use other Java 9 APIs when implementing a Java 9 API (e.g., `super.completeAsync`) - Maybe some small performance by not needing e.g., `maybeMakeContextAware`, `supplyAsync` for completion of current future - Possible readability improvement by being able to use `@Override` and otherwise IDE-awareness of the API level Disadvantage - If user shades Armeria into non-mrjar and runs on newer Java, newer APIs wouldn't function correctly
… source sets.
Now that we are using mrjar for version-specific implementations, we can use it for the recent change for completablefuture to support Java 9, and future ones to support Java 12.
Advantages
super.completeAsync
)maybeMakeContextAware
,supplyAsync
for completion of current future@Override
and otherwise IDE-awareness of the API levelDisadvantage
I think shading is advanced enough that the disadvantage is ok but wondering what everyone thinks.