Add experimental nio-transport plugin with http #26295
Conversation
| @@ -245,6 +245,11 @@ public void apply(Settings value, Settings current, Settings previous) { | |||
| HttpTransportSettings.SETTING_HTTP_MAX_HEADER_SIZE, | |||
| HttpTransportSettings.SETTING_HTTP_MAX_INITIAL_LINE_LENGTH, | |||
| HttpTransportSettings.SETTING_HTTP_RESET_COOKIES, | |||
| HttpTransportSettings.SETTING_HTTP_TCP_NO_DELAY, | |||
There was a problem hiding this comment.
can you make the move of these settings a seperate PR. This would help to keep this one focused
There was a problem hiding this comment.
Opened here: #26310
| @@ -39,7 +39,7 @@ | |||
| import java.util.Set; | |||
| import java.util.stream.Collectors; | |||
|
|
|||
| public class Netty4HttpRequest extends RestRequest { | |||
| class Netty4HttpRequest extends RestRequest { | |||
There was a problem hiding this comment.
can it also be final in that case?
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.TimeoutException; | ||
|
|
||
| public class ESChannelPromise implements ActionListener<NioChannel>, ChannelPromise { |
| import java.util.LinkedList; | ||
| import java.util.Queue; | ||
|
|
||
| class ESEmbeddedChannel extends EmbeddedChannel { |
There was a problem hiding this comment.
can you quickly explain what we are doing here?
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class NioPlugin extends Plugin implements NetworkPlugin { |
There was a problem hiding this comment.
I wonder if we should add a bootstrap check to make sure nobody uses this in production just yet? ie a check that always fails with a good explain error
There was a problem hiding this comment.
I added a check.
| ((Releasable) content).close(); | ||
| } | ||
| if (releaseBytesStreamOutput) { | ||
| bytesOutputOrNull().close(); |
There was a problem hiding this comment.
this asks for an NPE given the title of the method? Should we maybe use mutliple try finally blocks just in case one of the close methods throws?
| @Override | ||
| public Method method() { | ||
| HttpMethod httpMethod = request.method(); | ||
| if (httpMethod == HttpMethod.GET) |
There was a problem hiding this comment.
this looks like it should be a switch statement or an EnumMap?
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { |
There was a problem hiding this comment.
unneeded default to size() == 0
| } | ||
|
|
||
| @Override | ||
| public boolean containsValue(Object value) { |
| } | ||
|
|
||
| final FullHttpRequest copy = | ||
| new DefaultFullHttpRequest( |
There was a problem hiding this comment.
maybe we can use a single line here?
Is it a good idea for a production plugin to depend on the test framework? Even if it is experimental I think that is a thing we should avoid. I suppose if we have a bootstrap check to make sure no one uses it in production then we can depend on the test framework until we drop it. |
|
|
||
| dependencies { | ||
| compile project(path: ':modules:transport-netty4', configuration: 'runtime') | ||
| compile project(path: ':test:framework', configuration: 'runtime') |
There was a problem hiding this comment.
What is it in the test framework needed here?
There was a problem hiding this comment.
The nio transport classes are in test:framework. Selectors, channels, etc.
There was a problem hiding this comment.
I thought they were going to move into this plugin?
There was a problem hiding this comment.
When I spoke to @s1monw about this last week, his preference was to leave the tcp version of the nio transport in test:framework for now for for testing.
|
I want to comment on this to say - I spoke to @s1monw about his suggested changes to:
The problem here is that these classes are just duplicates of the netty module classes (with slight modifications to make them work with nio functionality). His suggestions would also apply to the netty classes. So we decided to hold of those changes. I might submit a follow-up PR that applies these suggestions to both netty / nio. |
|
@tbrooks8 are we ready to move here? Shall I take another look? |
|
@s1monw yes. I made changes based on all your feedback that was specific for nio. I can submit a follow up PR for the changes that apply to both this and the netty module. |
|
This PR has been opened for a while and I think is ready for more reviews as I have address @s1monw concerns. Although I do have a concern about how this will work. @jasontedor I guess that when this gets merged I will put the http plugin part on 7.0 and back port the nio transport changes only to 6.X? As I think you said you wanted the plugin part on 7.0 only? |
|
I don't need any reviews on this PR currently. There has been a number of changes to the various nio classes. This PR will need some conflicts resolved before it is ready to be reviewed again. |
|
I am closing this PR. It is well out of date with updates to nio. I will eventually use some of this work to submit a separate PR for http. |
This commit adds an nio-transport plugin. It builds upon the nio
transport work in test:framework. However, it adds http functionality.
The http functionality is added by adapting a netty embedded channel.
For now, the bulk of the nio networking and tcp transport will remain in
test:framework. This allows it to be used as a tcp transport for
internal clusters.
This plugin is unique in that it depends on test:framework for the nio
work and the netty4 transport module for http (cors, pipeling, etc)
handlers.