Skip to content

Implemented doThen method to chain ConnectionPoolListener - #5166

Closed
asontireddy wants to merge 1 commit into
line:mainfrom
asontireddy:asontireddy-add_doThen_in_ConnectionPoolListener
Closed

Implemented doThen method to chain ConnectionPoolListener#5166
asontireddy wants to merge 1 commit into
line:mainfrom
asontireddy:asontireddy-add_doThen_in_ConnectionPoolListener

Conversation

@asontireddy

Copy link
Copy Markdown

Motivation:

By implementing the doThen method, we can able to chain the ConnectionPoolListeners, which makes it easier to use

Modifications:

  • created a new method doThen() which accepts ConnectionPoolListener and returns the new ConnectionPoolListener

Result:

  • Closes #5159
  • User can now chain more than one listener

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@asontireddy

Copy link
Copy Markdown
Author

@trustin Can you please go through the changes I have made and let me know if I am going in the right direction? Also, the project is building fine locally but I am getting some errors when I run existing tests, Can you please help me resolve those errors, so that I can write a unit test for this change?

@asontireddy asontireddy changed the title Implemented doThen method to chain **ConnectionPoolListener** Implemented doThen method to chain ConnectionPoolListener Sep 3, 2023
@asontireddy

Copy link
Copy Markdown
Author

Hi Guys, also I signed the CLA, but it is still showing as pending, Is there anything else that I need to do in that area so that action passes?

@jrhee17

jrhee17 commented Sep 4, 2023

Copy link
Copy Markdown
Contributor

Hi Guys, also I signed the CLA, but it is still showing as pending, Is there anything else that I need to do in that area so that action passes?

Did you sign the CLA with the same email used for the commit?

@jrhee17 jrhee17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Left some minor comments 👍

return this;
}

default ConnectionPoolListener doThen(ConnectionPoolListener anotherConnectionPoolListener) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
default ConnectionPoolListener doThen(ConnectionPoolListener anotherConnectionPoolListener) {
default ConnectionPoolListener andThen(ConnectionPoolListener anotherConnectionPoolListener) {

return new ConnectionPoolListener() {
@Override
public void connectionOpen(SessionProtocol protocol, InetSocketAddress remoteAddr, InetSocketAddress localAddr, AttributeMap attrs) throws Exception {
anotherConnectionPoolListener.connectionOpen(protocol, remoteAddr, localAddr, attrs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the expectation is that this#connectionOpen is called, and then anotherConnectionPoolListener#connectionOpen is called (hence "chained").

Can you also add a test to confirm this behavior?

Suggested change
anotherConnectionPoolListener.connectionOpen(protocol, remoteAddr, localAddr, attrs);
connectionOpen(protocol, remoteAddr, localAddr, attrs);
anotherConnectionPoolListener.connectionOpen(protocol, remoteAddr, localAddr, attrs);

@codecov

codecov Bot commented Sep 4, 2023

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (863e27c) to head (bb3b110).
Report is 688 commits behind head on main.

Files with missing lines Patch % Lines
...inecorp/armeria/client/ConnectionPoolListener.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5166   +/-   ##
=========================================
  Coverage     74.25%   74.25%           
- Complexity    19825    19828    +3     
=========================================
  Files          1699     1699           
  Lines         73046    73052    +6     
  Branches       9357     9357           
=========================================
+ Hits          54239    54245    +6     
- Misses        14371    14374    +3     
+ Partials       4436     4433    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Ivan-Montes

Copy link
Copy Markdown
Contributor

Hi @jrhee17, I would like to contribute to this solution if you agree.

Ivan-Montes added a commit to Ivan-Montes/armeria that referenced this pull request Apr 16, 2025
---

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
@minwoox

minwoox commented Apr 17, 2025

Copy link
Copy Markdown
Contributor

Let me close this PR since it's not updated and superseded by #6207

@minwoox minwoox closed this Apr 17, 2025
Ivan-Montes added a commit to Ivan-Montes/armeria that referenced this pull request Apr 17, 2025
---

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
jrhee17 pushed a commit that referenced this pull request May 12, 2025
Related: #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 #5159
- Supersedes #5166 This PR replaces the changes proposed in #5166, which
is now outdated.
- Now, we could chain more than one listener
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ConnectionPoolListener.andThen(ConnectionPoolListener)

7 participants