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 poolMaxIdleTime configuration option to TCP appenders #49

Merged
merged 1 commit into from
May 31, 2020

Conversation

joelittlejohn
Copy link
Contributor

When connections remain open without being used, they may be
considered stale by the server and closed. This can lead to log messages
failing with

java.lang.SocketException: broken pipe

This commit introduces a maximum 'idle time' for pooled connections. If
a connection has not been used within that time, it will be considered
stale and not reused. By default, this new feature is disabled.

I'm interested to hear your thoughts on this PR, and happy to make changes and adjustments if you need me to. We have been experiencing a bug where many of our log messages failed to arrive in graylog and tracked the problem down to the server ELB max idle time. This PR is intended to help us recycle connections if they have not been used recently, but to avoid recycling them if they have been used recently.

@joelittlejohn
Copy link
Contributor Author

joelittlejohn commented May 11, 2020

Regarding the failed quality check, I followed the convention already used in AbstractPooledObject. There is no way to explicitly specify package-private access. I can either change these to protected, or leave it alone so that it's consistent with the existing code.

Copy link
Owner

@osiegmar osiegmar left a comment

Choose a reason for hiding this comment

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

Thank you very much for this contribution! I think it is a very reasonable addition to this library. Regarding the failed quality check – you can safely ignore it as I was just playing with Codacy a bit and its configuration does not match the checkstyle/spotbugs rules within this project. I added a few comments that I'd like to ask you to change in order I can merge this PR.

@joelittlejohn
Copy link
Contributor Author

Hi @osiegmar, thanks for looking at this! I've force pushed to update my branch as per your review comments.

If you want to see the exact changes I made, they're here:

https://github.com/joelittlejohn/logback-gelf/compare/5b4168d..0779a34

When connections remain open without being used, they may be
considered stale by the server and closed. This can lead to log messages
failing with

java.lang.SocketException: broken pipe

This commit introduces a maximum 'idle time' for pooled connections. If
a connection has not been used within that time, it will be considered
stale and not reused. By default, this new feature is disabled.
@osiegmar
Copy link
Owner

Thanks a lot!

@osiegmar osiegmar closed this May 31, 2020
@osiegmar osiegmar reopened this May 31, 2020
@osiegmar osiegmar merged commit 46d54d7 into osiegmar:master May 31, 2020
@osiegmar
Copy link
Owner

BTW what value (time interval) have you used for getting rid of your problem? Have you disabled reconnectInterval then?

@joelittlejohn
Copy link
Contributor Author

We found that our connections were considered stale after somewhere between 15 to 20 seconds of idle time, so to be on the safe side we're using a max idle time of 10 seconds. We're still using reconnectInterval although it's probably not necessary with the max idle time in place.

@osiegmar osiegmar mentioned this pull request Jun 12, 2020
@osiegmar osiegmar added this to the 4.0.0 milestone May 22, 2021
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.

2 participants