-
Notifications
You must be signed in to change notification settings - Fork 972
Add andThen method to ConnectionPoolListener
#6207
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
Add andThen method to ConnectionPoolListener
#6207
Conversation
core/src/test/java/com/linecorp/armeria/client/ConnectionPoolListenerTest.java
Outdated
Show resolved
Hide resolved
| } finally { | ||
| nextConnectionPoolListener.connectionClosed(protocol, remoteAddr,localAddr,attrs); | ||
| } | ||
| } |
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.
Question) What do you think of also calling close as well?
e.g.
@Override
public void close() {
try {
ConnectionPoolListener.super.close();
} finally {
nextConnectionPoolListener.close();
}
}On side note, I think we're missing a call to ConnectionPoolMetrics#close from MetricCollectingConnectionPoolListener; I think we'll fix that separately
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 for the suggestion! I'll implement that.
ikhoon
left a comment
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, @Ivan-Montes!
core/src/main/java/com/linecorp/armeria/client/ConnectionPoolListener.java
Show resolved
Hide resolved
--- Related: line#5159 **Motivation:** Add `andThen` method to `ConnectionPoolListener` to chain more than one listener **Modifications:** - In `ConnectionPoolListener`, add `andThen` method. - In `ConnectionPoolListenerTest`, add test **Result:** - Closes line#5159 - Supersedes line#5166 This PR replaces the changes proposed in line#5166, which is now outdated. - Now, we could chain more than one listener
f23733e to
dec0883
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6207 +/- ##
============================================
+ Coverage 74.46% 74.60% +0.14%
- Complexity 22234 22466 +232
============================================
Files 1963 1972 +9
Lines 82437 83003 +566
Branches 10764 10798 +34
============================================
+ Hits 61385 61923 +538
- Misses 15918 15936 +18
- Partials 5134 5144 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
**Modifications:** - In `ConnectionPoolListener`, implement `close` method within the `andThen` method. - In `ConnectionPoolListener`, annotate the `andThen` method with `@UnstableApi`. - In `ConnectionPoolListenerTest`, add `throws Exception` to the method signature
dec0883 to
896c7a9
Compare
|
Thanks for the feedback! I've implemented the suggested changes. Please let me know if there's anything else that needs to be updated. |
|
ping @minwoox |
minwoox
left a comment
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, @Ivan-Montes 👍
Thank you team!! |
Related: #5159
Motivation:
Add
andThenmethod toConnectionPoolListenerto chain more than one listenerModifications:
ConnectionPoolListener, addandThenmethod.ConnectionPoolListenerTest, add testResult:
ConnectionPoolListener.andThen(ConnectionPoolListener)#5159