You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "elkhand (via GitHub)" <gi...@apache.org> on 2023/09/22 14:00:58 UTC
[GitHub] [flink] elkhand opened a new pull request, #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
elkhand opened a new pull request, #23452:
URL: https://github.com/apache/flink/pull/23452
[FLINK-32884](https://issues.apache.org/jira/browse/FLINK-32884) PyFlink remote execution should support URLs with paths and https scheme
## What is the purpose of the change
This PR handles including customHeaders in messageHeaders when a Python client is sending requests. This will allow sending auth tokens with the request.
## Brief change log
- send messageHeaders which are decorated with customHeaders
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
This change is already covered by existing tests, such as `flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java#testRESTManualConfigurationOverride`
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper:no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23452:
URL: https://github.com/apache/flink/pull/23452#issuecomment-1731485499
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "44f8e48e6ca37c2fcd91f52d7b01c2b3d109b038",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "44f8e48e6ca37c2fcd91f52d7b01c2b3d109b038",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 44f8e48e6ca37c2fcd91f52d7b01c2b3d109b038 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] tweise commented on a diff in pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "tweise (via GitHub)" <gi...@apache.org>.
tweise commented on code in PR #23452:
URL: https://github.com/apache/flink/pull/23452#discussion_r1337609425
##########
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java:
##########
@@ -909,6 +910,17 @@ void testSubmitJobAndWaitForExecutionResult() throws Exception {
final AtomicBoolean firstSubmitRequestFailed = new AtomicBoolean(false);
failHttpRequest =
(messageHeaders, messageParameters, requestBody) -> {
+ if (messageHeaders instanceof CustomHeadersDecorator) {
Review Comment:
This is a bit hard to follow. Shouldn't we expect to find `CustomHeadersDecorator` and throw an error otherwise? If not then the bug you are trying to fix could be re-introduced w/o the test breaking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] tweise commented on a diff in pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "tweise (via GitHub)" <gi...@apache.org>.
tweise commented on code in PR #23452:
URL: https://github.com/apache/flink/pull/23452#discussion_r1337637880
##########
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java:
##########
@@ -909,6 +910,17 @@ void testSubmitJobAndWaitForExecutionResult() throws Exception {
final AtomicBoolean firstSubmitRequestFailed = new AtomicBoolean(false);
failHttpRequest =
(messageHeaders, messageParameters, requestBody) -> {
+ if (messageHeaders instanceof CustomHeadersDecorator) {
Review Comment:
My question is why the unwrapping is conditional? Isn't it an error if `messageHeaders` is not double wrapped? If so, then the test should check that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] elkhand commented on a diff in pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "elkhand (via GitHub)" <gi...@apache.org>.
elkhand commented on code in PR #23452:
URL: https://github.com/apache/flink/pull/23452#discussion_r1337654798
##########
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java:
##########
@@ -909,6 +910,17 @@ void testSubmitJobAndWaitForExecutionResult() throws Exception {
final AtomicBoolean firstSubmitRequestFailed = new AtomicBoolean(false);
failHttpRequest =
(messageHeaders, messageParameters, requestBody) -> {
+ if (messageHeaders instanceof CustomHeadersDecorator) {
Review Comment:
I see your point. I removed all checks, as now we will always wrap messageHeaders into `CustomHeadersDecorator(new UrlPrefixDecorator(messageHeaders))` before sending the request, thus we do not need those condition checks anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] elkhand commented on pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "elkhand (via GitHub)" <gi...@apache.org>.
elkhand commented on PR #23452:
URL: https://github.com/apache/flink/pull/23452#issuecomment-1731705336
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] tweise merged pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "tweise (via GitHub)" <gi...@apache.org>.
tweise merged PR #23452:
URL: https://github.com/apache/flink/pull/23452
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] elkhand commented on a diff in pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "elkhand (via GitHub)" <gi...@apache.org>.
elkhand commented on code in PR #23452:
URL: https://github.com/apache/flink/pull/23452#discussion_r1337629441
##########
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java:
##########
@@ -909,6 +910,17 @@ void testSubmitJobAndWaitForExecutionResult() throws Exception {
final AtomicBoolean firstSubmitRequestFailed = new AtomicBoolean(false);
failHttpRequest =
(messageHeaders, messageParameters, requestBody) -> {
+ if (messageHeaders instanceof CustomHeadersDecorator) {
Review Comment:
We are interested in `messageHeaders` itself, and it is double wrapped in `CustomHeadersDecorator(new UrlPrefixDecorator(messageHeaders))`. The test was checking for instance type of `messageHeaders`, which this code is trying to do.
`RestClusterClient.sendRequest()` function internally calls `RestClient.sendRequest()` function which accesses messageHeaders and customHeaders to construct httpRequest, and it is not accessed anywhere else. That part in RestClient is already updated to work with decorated messageHeaders:
https://github.com/apache/flink/pull/22816/files#diff-d6cdab55fefade687bd62fb086b6fa85aa4f8c91a745e8ec406fb8ac8982b9e1R440
https://github.com/apache/flink/pull/22556/files#diff-d6cdab55fefade687bd62fb086b6fa85aa4f8c91a745e8ec406fb8ac8982b9e1R358
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] elkhand commented on a diff in pull request #23452: [FLINK-32884] [flink-clients] Sending messageHeaders with decorated customHeaders for PyFlink client
Posted by "elkhand (via GitHub)" <gi...@apache.org>.
elkhand commented on code in PR #23452:
URL: https://github.com/apache/flink/pull/23452#discussion_r1337654798
##########
flink-clients/src/test/java/org/apache/flink/client/program/rest/RestClusterClientTest.java:
##########
@@ -909,6 +910,17 @@ void testSubmitJobAndWaitForExecutionResult() throws Exception {
final AtomicBoolean firstSubmitRequestFailed = new AtomicBoolean(false);
failHttpRequest =
(messageHeaders, messageParameters, requestBody) -> {
+ if (messageHeaders instanceof CustomHeadersDecorator) {
Review Comment:
I see your point. I removed all checks, as now we will always wrap messageHeaders into `CustomHeadersDecorator(new UrlPrefixDecorator(messageHeaders))` before sending the request, thus we do not need those condition checks anymore. UrlPrefixDecorator is needed for full path support in JM running on K8S and behind some ingress. CustomHeadersDecorator is needed for adding Auth token related headers into httpRequest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org