You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "gaborgsomogyi (via GitHub)" <gi...@apache.org> on 2023/04/18 14:30:49 UTC
[GitHub] [flink] gaborgsomogyi opened a new pull request, #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
gaborgsomogyi opened a new pull request, #22420:
URL: https://github.com/apache/flink/pull/22420
## What is the purpose of the change
At the moment it's not possible to add `flink-s3-fs-hadoop` and `flink-s3-fs-presto` plugins at the same time.
## Brief change log
* Now both the plugins are having a provider with a different name
* Fixed bad error message in the `DefaultDelegationTokenManager`
* Added a warning message when multiple providers are loaded with the same prefix.
## Verifying this change
Existing + additional unit tests.
## 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: yes
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513312954
There is a workaround if no tokens are expected: `security.delegation.token.provider.s3.enabled=false`
--
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] mbalassi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513272251
Let us make sure to backport this to 1.17.1, @gaborgsomogyi.
--
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] mbalassi commented on a diff in pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on code in PR #22420:
URL: https://github.com/apache/flink/pull/22420#discussion_r1170863184
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/DefaultDelegationTokenManager.java:
##########
@@ -203,6 +211,21 @@ static void checkProviderAndReceiverConsistency(
LOG.info("Provider and receiver instances are consistent");
}
+ @VisibleForTesting
+ static void checkSamePrefixedProviders(
+ Map<String, DelegationTokenProvider> providers, List<String> warnings) {
+ Set<String> providerPrefixes = new HashSet<>();
+ for (String name : providers.keySet()) {
+ String[] split = name.split("-");
+ if (!providerPrefixes.add(split[0])) {
+ String msg =
+ String.format(
+ "Multiple providers loaded with the same prefix: %s", split[0]);
Review Comment:
Let us add a bit more context to this, I like this message as the beginning but let us also add:
`Please consider using only one of the following providers: <list of clashing providerNames>, the current configuration might lead to unintended consequences.`
We should also add some content to the flink docs about these unintended consequences. :)
--
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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1514308039
In tests and example execution it's fine to add all the plugins to the classpath just for testing purposes but discouraged to do it in production because of the written down unintended consequences.
--
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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513248254
cc @mbalassi @gyfora
--
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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513283108
OK, when the PR is approved and merged I'm intended to create a PR against 1.17.
--
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] MartijnVisser commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513298938
> Let us make sure to backport this to 1.17.1, @gaborgsomogyi.
I've bumped the ticket to a Critical as well, given that the impact is likely to be quite big. We should consider thinking about creating a 1.17.1 soon
--
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] mbalassi commented on a diff in pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on code in PR #22420:
URL: https://github.com/apache/flink/pull/22420#discussion_r1170863184
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/DefaultDelegationTokenManager.java:
##########
@@ -203,6 +211,21 @@ static void checkProviderAndReceiverConsistency(
LOG.info("Provider and receiver instances are consistent");
}
+ @VisibleForTesting
+ static void checkSamePrefixedProviders(
+ Map<String, DelegationTokenProvider> providers, List<String> warnings) {
+ Set<String> providerPrefixes = new HashSet<>();
+ for (String name : providers.keySet()) {
+ String[] split = name.split("-");
+ if (!providerPrefixes.add(split[0])) {
+ String msg =
+ String.format(
+ "Multiple providers loaded with the same prefix: %s", split[0]);
Review Comment:
Let us add a bit more context to this, I like this message as the beginning but let us also add:
`This might lead to unintended consequences, please consider using only one of them.`
We should also add some content to the flink docs about these unintended consequences. :)
--
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 #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1513259952
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "d641e1e354ae128385796f5fe975a293afaa6f0f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "d641e1e354ae128385796f5fe975a293afaa6f0f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d641e1e354ae128385796f5fe975a293afaa6f0f 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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1514312632
I've also seen patterns that users are just adding plugins to the main classpath of the job as normal maven/gradle dependency which is also bad practice. Such cases the plugins are loaded by the Flink's main classloader which can cause dependency issues easily. Just for the record the good example can be found [here](https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/filesystems/s3/#hadooppresto-s3-file-systems-plugins) how a plugin must be added in a proper way.
--
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] gaborgsomogyi commented on pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #22420:
URL: https://github.com/apache/flink/pull/22420#issuecomment-1514319412
If one wants to execute job source code `locally` together with plugins then I suggest the following:
```
public static void main(String[] args) throws Exception {
Configuration configuration = new Configuration();
...
PluginManager pluginManager = PluginUtils.createPluginManagerFromRootFolder(configuration);
FileSystem.initialize(configuration, pluginManager);
StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment(configuration);
...
}
```
This loads the plugins in separate classloader.
--
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] mbalassi merged pull request #22420: [FLINK-31839][filesystems] Fix flink-s3-fs-hadoop and flink-s3-fs-presto plugin collision
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi merged PR #22420:
URL: https://github.com/apache/flink/pull/22420
--
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