-
Notifications
You must be signed in to change notification settings - Fork 355
added useLocalAuthorityResolver() to ServerBootstrap and AsyncServerBootStrap classes #556
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
added useLocalAuthorityResolver() to ServerBootstrap and AsyncServerBootStrap classes #556
Conversation
garydgregory
left a comment
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.
Please see my comments.
httpcore5/src/main/java/org/apache/hc/core5/http/impl/bootstrap/AsyncServerBootstrap.java
Outdated
Show resolved
Hide resolved
httpcore5/src/main/java/org/apache/hc/core5/http/impl/bootstrap/AsyncServerBootstrap.java
Outdated
Show resolved
Hide resolved
httpcore5/src/main/java/org/apache/hc/core5/http/impl/bootstrap/ServerBootstrap.java
Outdated
Show resolved
Hide resolved
httpcore5/src/main/java/org/apache/hc/core5/http/impl/bootstrap/ServerBootstrap.java
Outdated
Show resolved
Hide resolved
8fb1d8e to
bf7d524
Compare
|
Hello @garydgregory, seems this request is in deadlock, so let me explain the background of my request. I migrated a project from httpcore4 to httpcore5 just recently and when it didn't work, I had to dive into the httpcore5 sources to understand what was going wrong and that I need to create a custom RequestRouter with LOCAL_AUTHORITY_RESOLVER scheme to get the same behavior as before with httpcore4 just using simple ServerBootstrap methods. I might be the last on earth migrating an ancient httpcore4 application, but in this scenario and I guess also in others where the host name shall be irrelevant (and which is the reason why the LOCAL_AUTHORITY_RESOLVER scheme exists in httpcore5), it is quite a pain IMO to build the custom RequestRouter in the application itself, especially because this isn't really very well documented. But in the end, it's not my decision, so if you decide it's not worth the API extension, then just reject the pull request and nobody will get hurt either. |
|
@cdw8j Please address my comments if you want this pull request merged. |
ok2c
left a comment
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.
@cdw8j My apologies. I actually did not submit my review last time.
Please see my comments,
| } | ||
|
|
||
| /** | ||
| * Create {@link RequestRouter} with LOCAL_AUTHORITY_RESOLVER (default: IGNORE_PORT_AUTHORITY_RESOLVER). |
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.
@cdw8j Please do not mention LOCAL_AUTHORITY_RESOLVER and IGNORE_PORT_AUTHORITY_RESOLVER. This is an implementation details and it should be mentioned in javadocs
| * | ||
| * @since 5.4 | ||
| */ | ||
| public final ServerBootstrap useLocalAuthorityResolver(final boolean localAuthorityResolver) { |
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.
@cdw8j Please use a better name for the method. #routeToLocalAuthority for example.
|
@ok2c Re-thinking this whole thing after it has been idling a bit in my head, I wonder if it wouldn't actually make more sense to implement setter methods in the What do you think? Should I create a separate pull request for this option and you decide which is preferrable? |
@cdw8j I personally like this idea much better. If you do it fast enough there is a a good chance it will make it into the coming 5.4-alpha1 release |
|
@ok2c not too much of a challenge :) see pull request #565 |
|
Superseded by #565 |
(Follow-up to pull request #555 which I messed up while trying to rebase it from 5.3.x to master.)
I have added a convenience method to the ServerBootstrap and AsyncServerBootstrap classes that configures the RequestRouter to use the LOCAL_AUTHORITY_RESOLVER scheme. While it is possible to create an HttpServer with such a custom RequestRouter within the application, this results in really messy and complicated code, which can easily be spared with this very simple convenience method.