-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-23272][web] Add custom netty HTTP request inbound/outbound handlers #16463
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 743a2a8 (Mon Jul 12 07:14:32 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
Added some minor comments, also please add a test that validates that handlers are actually added and in the correct order according to their priority.
We can use some test/mock handlers that we can easily validate :)
...me/src/main/java/org/apache/flink/runtime/io/network/netty/InboundChannelHandlerFactory.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/flink/runtime/io/network/netty/OutboundChannelHandlerFactory.java
Outdated
Show resolved
Hide resolved
743a2a8
to
758e058
Compare
As a start resolved the conflict. |
The tests are not yet testing the netty chain so discovering how is it possible to do that w/o horror complex code... |
e99c717
to
2f1be8a
Compare
@gyfora added proper tests which are testing the loaded factories behavior but this has increased the PR size significantly. I don't think I can come up w/ more simple tests. If somebody has any idea please share. |
2f1be8a
to
8743ec5
Compare
@flinkbot run azure |
@flinkbot approve all |
|
@flinkbot run azure |
|
Filed FLINK-23665 and FLINK-23666. |
@gyfora it would be nice to update the JIRA tickets when merging a PR. Otherwise it is hard to find the paper trail when looking things up. Concretely, this PR seems to reference FLINK-21108 in the commit but the PR title states that it is FLINK-23272. Moreover, FLINK-21108 has been closed as a duplicate more than a month ago. |
Yea, we updated the ticket but unfortunately wrong ticket was referenced in the commit. We were so preoccupied with the CI problems that we did not notice this. :( |
Np. I think it should be fine if you update the respective Jira tickets accordingly. Thanks! |
What is the purpose of the change
At the moment there is no possibility to influence netty HTTP inbound/outbound requests. This could be useful from many perspectives. A good example (amongst others) is authentication/authorization. Here are the exact examples:
In this PR I've added 2 generic APIs in order to handle inbound and outbound HTTP requests.
Please see FLIP-181 for further details.
Brief change log
InboundChannelHandlerFactory
OutboundChannelHandlerFactory
TestRestServerEndpoint
andRestClient
are now getting FlinkConfiguration
instances.Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yes, introduced 2@Experimental
APIsDocumentation