Support http read timeouts for transport-nio #41466
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tim-Brooks
added
>non-issue
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed |
plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/HttpReadWriteHandler.java
Show resolved
Hide resolved
plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java
Outdated
Show resolved
Hide resolved
plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/HttpReadWriteHandler.java
Outdated
Show resolved
Hide resolved
s1monw
reviewed
Apr 25, 2019
plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/HttpReadWriteHandler.java
Show resolved
Hide resolved
plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/HttpReadWriteHandler.java
Outdated
Show resolved
Hide resolved
plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java
Outdated
Show resolved
Hide resolved
|
@s1monw @original-brownbear - I made your changes. Thanks. |
original-brownbear
suggested changes
Apr 25, 2019
| * timestamp, see {@link #absoluteTimeInMillis()}. | ||
| */ | ||
| public long relativeTimeInNanos() { | ||
| return TimeValue.msecToNSec(cachedTimeThread.relativeTimeInMillis()); |
There was a problem hiding this comment.
@tbrooks8 I'm not sure we should add this. The underlying logic as it is coded up now isn't nano second precise.
We could adjust CachedTimeThread to return nano second precision times and only add the rounding to ms in the existing relativeTimeInMillis method?
But then again, maybe we don't need ns for the http timeout in the first place and can just work with ms precision and not add more complication to the ThreadPool class?
There was a problem hiding this comment.
We could adjust CachedTimeThread to return nano second precision times and only add the rounding to ms in the existing relativeTimeInMillis method?
I think this is the correct approach, but I hesitated to change it in this PR as the behavior of the timer tick thread might be of concern to other people. Anyway, I will make that change and we can discuss.
There was a problem hiding this comment.
Anyway, I made this change.
- We need nanoseconds.
TaskSchedulerdeals in nanoseconds. - If we do the conversion outside of the
ThreadPoolwe depend on the fact thatThreadPoolwill always return a relative millis value that can be successfully converted to nanoseconds. Obviously this is the case as the value originates from nanotime, but I prefer to not depend on that fact. Which is why I prefer thatThreadPoolreturns nanotime (similar to the fact that most Java relative time calculations are made using nanotime). - The cost of the change is division every time you want millis. I assume this is fine, but obviously the entire point of this tick thread is to replace a
System.nanotimecall with a volatile read at the cost of resolution. So maybe there is some objection. We could also store a precalculated version in millis.
original-brownbear
approved these changes
Apr 25, 2019
|
thanks @tbrooks8 LGTM now |
|
/cc @andrershov that we can as part of the netty vs nio benchmarking check whether enabling http read timeouts have any effect either way. |
ywelsch
approved these changes
Apr 26, 2019
plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/HttpReadWriteHandlerTests.java
Outdated
Show resolved
Hide resolved
.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java
Show resolved
Hide resolved
s1monw
approved these changes
Apr 30, 2019
server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/2 |
Tim-Brooks
added a commit
to Tim-Brooks/elasticsearch
that referenced
this pull request
May 1, 2019
This is related to elastic#27260. Currently there is a setting http.read_timeout that allows users to define a read timeout for the http transport. This commit implements support for this functionality with the transport-nio plugin. The behavior here is that a repeating task will be scheduled for the interval defined. If there have been no requests received since the last run and there are no inflight requests, the channel will be closed.
akhil10x5
pushed a commit
to akhil10x5/elasticsearch
that referenced
this pull request
May 2, 2019
This is related to elastic#27260. Currently there is a setting http.read_timeout that allows users to define a read timeout for the http transport. This commit implements support for this functionality with the transport-nio plugin. The behavior here is that a repeating task will be scheduled for the interval defined. If there have been no requests received since the last run and there are no inflight requests, the channel will be closed.
Tim-Brooks
added a commit
that referenced
this pull request
May 2, 2019
This is related to #27260. Currently there is a setting http.read_timeout that allows users to define a read timeout for the http transport. This commit implements support for this functionality with the transport-nio plugin. The behavior here is that a repeating task will be scheduled for the interval defined. If there have been no requests received since the last run and there are no inflight requests, the channel will be closed.
gurkankaymak
pushed a commit
to gurkankaymak/elasticsearch
that referenced
this pull request
May 27, 2019
This is related to elastic#27260. Currently there is a setting http.read_timeout that allows users to define a read timeout for the http transport. This commit implements support for this functionality with the transport-nio plugin. The behavior here is that a repeating task will be scheduled for the interval defined. If there have been no requests received since the last run and there are no inflight requests, the channel will be closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to #27260. Currently there is a setting
http.read_timeoutthat allows users to define a read timeout for thehttp transport. This commit implements support for this functionality
with the transport-nio plugin. The behavior here is that a repeating
task will be scheduled for the interval defined. If there have been
no requests received since the last run and there are no inflight
requests, the channel will be closed.