-
Notifications
You must be signed in to change notification settings - Fork 986
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
[UNDERTOW-2039] Convert AbstractFramedChannel.AWAIT_WRITABLE_TIMEOUT to nanoseconds #1577
Conversation
Hi @fl4via , could you look at this PR if it is correctly implemented? |
@@ -73,8 +73,8 @@ public abstract class AbstractFramedStreamSinkChannel<C extends AbstractFramedCh | |||
|
|||
|
|||
static { | |||
final int defaultAwaitWritableTimeout = 600000; | |||
int await_writable_timeout = AccessController.doPrivileged((PrivilegedAction<Integer>) () -> Integer.getInteger("io.undertow.await_writable_timeout", defaultAwaitWritableTimeout)); | |||
final long defaultAwaitWritableTimeout = 600000000000L; |
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.
This is not readable, lets create sustainable software, e.g. replace with:
final long defaultAwaitWritableTimeout = TimeUnit.MINUTES.toNanos(10);
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.
You are right, I fixed it.
Also, I am unsure about reading io.undertow.await_writable_timeout
property in nanoseconds. I would change it to seconds or minutes as well, and then parse it to nanoseconds. But I can't find where this property can be set.
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.
It's set as a system property.
final int defaultAwaitWritableTimeout = 600000; | ||
int await_writable_timeout = AccessController.doPrivileged((PrivilegedAction<Integer>) () -> Integer.getInteger("io.undertow.await_writable_timeout", defaultAwaitWritableTimeout)); | ||
final long defaultAwaitWritableTimeout = TimeUnit.MINUTES.toNanos(10); | ||
long await_writable_timeout = AccessController.doPrivileged((PrivilegedAction<Long>) () -> Long.getLong("io.undertow.await_writable_timeout", defaultAwaitWritableTimeout)); |
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.
This would break compatibility - users were setting io.undertow.await_writable_timeout
thinking they are setting milis, but now that value would be taken directly and treated to nanoseconds.
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 answer. I was thinking that maybe I could edit some documentation to mention the change of property for better readability to seconds or minutes. But for now, we can keep the system property in milliseconds and convert it to nanos after reading it.
@ropalka would be good idea to run CI as well ;-) you need to approve running. |
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.
LGTM
https://issues.redhat.com/browse/UNDERTOW-2039