-
Notifications
You must be signed in to change notification settings - Fork 301
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
PAYARA-546 Allow setting SO_KeepAlive on the server side. #3533
Conversation
@@ -105,10 +106,9 @@ | |||
private static final String PERSISTENT_SSL = "PERSISTENT_SSL"; | |||
|
|||
private static final int BACKLOG = 50; | |||
|
|||
private static final String SO_KEEPALIVE = "fish.payara.SOKeepAlive"; |
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.
I think the case of this property is so complicated, it will lead to issues.
May I suggest fish.payara.so-keepalive
jenkins test |
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.
Just one comment on top of @lprimak's current comment.
// Enable or disable SO_KEEPALIVE for the socket as required | ||
try { | ||
if (Boolean.getBoolean(IIOPSSLSocketFactory.SO_KEEPALIVE) && !socket.getKeepAlive()) { | ||
if (logger.isLoggable(Level.FINER)) { |
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.
I'm not a fan of this logger statement being so frequent. Feel free to ignore this if you disagree, but when this log level is enabled you'll likely get an absolute tonne of these messages. All except the first one (I think) will be unnecessary. It would possibly be better to add a method to check if it's changed and log if it does. This could by all means still be called from this method.
Set via a SystemProperty on all IIOP listeners.
Follow up PR (#3534) is a provisionary improvement that needs a fine-grained review as I'm not convinced that it'll work on multi-homed environments.