-
Notifications
You must be signed in to change notification settings - Fork 566
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
Issue 5416: Transfer configuration to connection providers. #5682
Conversation
This is actually still work in progress. I just need some feedback from @tomas-langer |
ecf6c8b
to
ac5ee78
Compare
nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Config.java
Outdated
Show resolved
Hide resolved
nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2ConnectionProvider.java
Show resolved
Hide resolved
...ebserver/webserver/src/main/java/io/helidon/nima/webserver/spi/ServerConnectionProvider.java
Show resolved
Hide resolved
nima/http2/webserver/src/main/java/io/helidon/nima/http2/webserver/Http2Connection.java
Outdated
Show resolved
Hide resolved
|
||
// Builds LoomServer instance including connectionProviders list. | ||
WebServer.Builder wsBuilder = WebServer.builder() | ||
.config(config.get("server")); |
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.
If you want to check that the configuration was propagated, you can have static methods on your custom connection provider, build the server, and then check that your connection provider obtained the correct setup.
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.
Well, yes, that's 1st part of the test. But it's not enough.
I would also like to know that those specific 2 values described in the issue got propagated into local Http2Settings instance of the Http2Connection.
.config(config.get("server")); | ||
|
||
// Call wsBuilder.connectionProviders() trough reflection | ||
Method connectionProviders = WebServer.Builder.class.getDeclaredMethod("connectionProviders",null); |
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 very hard to maintain, as it escapes refactoring.
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.
Unfortunately onnectionProviders() is not in the same package here. Theoretically I can create accessor in in tests of WebServer module where there is no problem to have the same package in the tests.
nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java
Outdated
Show resolved
Hide resolved
dd41723
to
17d98ab
Compare
9c35da8
to
e48f502
Compare
All builds/tests are finally green. Let's do final review & merge. |
e48f502
to
2185849
Compare
Rebase to check whether copyrights are ok on 2023. |
025cf9d
to
edc8a72
Compare
edc8a72
to
e7d7992
Compare
e7d7992
to
a921b8d
Compare
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
a921b8d
to
8577b6d
Compare
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Closing this PR, replaced by #5883 |
Signed-off-by: Tomáš Kraus tomas.kraus@oracle.com