Skip to content
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 Connection Pool Close configuration #389

Merged
merged 8 commits into from
Aug 28, 2024
Merged

Add Connection Pool Close configuration #389

merged 8 commits into from
Aug 28, 2024

Conversation

Gsantomaggio
Copy link
Member

Closes: #388

Closes: #388
Signed-off-by: Gabriele Santomaggio <[email protected]>
@Gsantomaggio
Copy link
Member Author

@jonnepmyra Can you please test it?

                ConnectionPoolConfig = new ConnectionPoolConfig()
                {
                    ConnectionCloseConfig = new ConnectionCloseConfig()
                    {
                        Policy = ConnectionClosePolicy.CloseWhenEmptyAndIdle,
                        IdleTime = TimeSpan.FromSeconds(60)
                    }
                }

Signed-off-by: Gabriele Santomaggio <[email protected]>
@jonnepmyra
Copy link
Contributor

Wow, that was fast :) , thank you!

I'm afraid that I will not be as fast ;), it may take a little longer than usual to test due to the holiday period, but I'll test as soon as possible!

@lukebakken lukebakken self-assigned this Jul 12, 2024
@lukebakken lukebakken self-requested a review July 12, 2024 15:58
* Dispose of pools when closing.
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

@Gsantomaggio I did a quick initial review, check out the commit I just made. There is a TODO to be addressed, maybe?

@Gsantomaggio
Copy link
Member Author

Thanks a lot @lukebakken

Here

// TODO should everything be closed and cleaned-up here?

You raised a good point and an old "problem" of this client:
When you close the StreamSystem, the producers and consumers created by StreamSystem won't be closed.

The behaviour is different from Java and Go, see for example, Go Stream Client

We should also add this behaviour to this client, but it will introduce a sort of breaking change. Can we think of a 1.9 version?

The change has to deal with the auto-reconnect feature and close all the producers and consumers in the right way to avoid triggering the auto-reconnection

Gsantomaggio and others added 2 commits July 25, 2024 14:33
@jonnepmyra
Copy link
Contributor

Hi,

I've tested this change and it looks great!

In my test case, I created short-lived consumers (create-consume-close) with a couple of seconds between each iteration.

Test results:

ConnectionClosePolicy.CloseWhenEmpty (as before this change):
Mean/avg consumer creation time: 9.88527 ms

ConnectionClosePolicy.CloseWhenEmptyAndIdle:
Mean/avg consumer creation time: 3.85261 ms

great work!

and close all pending connections when the pool is closed

Signed-off-by: Gabriele Santomaggio <[email protected]>
@Gsantomaggio
Copy link
Member Author

@lukebakken To avoid inconsistency, I added a check during the pool connection close only in case the policy is CloseWhenEmptyAndIdle

That is not perfect, but it could be enough for the moment.

Version 2.0, we can think of adopting the same Golang and Java behaviour:

Close all the Producers and Consumers when the System is closed. 

This is a breaking change that I want to avoid introducing in this version.

With the policy CloseWhenEmpty ( default ) won't change anything btw.

@jonnepmyra thoughts?

Signed-off-by: Gabriele Santomaggio <[email protected]>
@Gsantomaggio Gsantomaggio marked this pull request as ready for review August 27, 2024 10:11
@Gsantomaggio Gsantomaggio added this to the 1.8.8 milestone Aug 28, 2024
@Gsantomaggio Gsantomaggio merged commit b648639 into main Aug 28, 2024
2 checks passed
@Gsantomaggio Gsantomaggio deleted the pool_timeout branch August 28, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Connection Management for Short-Lived Consumers
3 participants