You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/05/15 12:11:27 UTC
[GitHub] [flink] kl0u opened a new pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
kl0u opened a new pull request #12170:
URL: https://github.com/apache/flink/pull/12170
This PR builds on top of https://github.com/apache/flink/pull/12143 and adds the functionality described in the JIRA. It makes mandatory for users of the `yarn.provided.lib.dirs` option to also include the `lib/`, `plugin/` and `flink-dist jar`.
If these are not included among the shared resources, Flink will throw an exception and will inform the user to not use this feature, but ship all the dependencies of the application, as it was done previously.
If these dirs are provided, then these are going to be used to execute the application and **NOT** what the user may have locally (e.g. different flink or log4j versions).
The reason for this requirement is to avoid unpleasant surprises with classloading issues. Alternatively we could say to go case-by-case and say if the file to be shipped has the same name as one in the shared directories, then do not ship it and use the remote. But this way of doing things transparently to the user oftentimes leads to unpleasant surprises that are difficult to debug.
## Brief change log
More of the changes are in the `YarnApplicationFileUploader`.
## Verifying this change
Partially covered by tests in the `YarnITCase` and also verified manually by me and @wangyang0918 .
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (**yes** / no / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (**yes** / 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218834
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"triggerType" : "PUSH"
}, {
"hash" : "4419cf15c0320b96afcb3d767322721007a7268f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445",
"triggerID" : "4419cf15c0320b96afcb3d767322721007a7268f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b774a5652648e7bd23cdc02356237809dd6931e7 UNKNOWN
* 4419cf15c0320b96afcb3d767322721007a7268f Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218834
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"triggerType" : "PUSH"
}, {
"hash" : "4419cf15c0320b96afcb3d767322721007a7268f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445",
"triggerID" : "4419cf15c0320b96afcb3d767322721007a7268f",
"triggerType" : "PUSH"
}, {
"hash" : "345706a3f30eea79d24f0bdb7c5f6aa66db47dee",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "345706a3f30eea79d24f0bdb7c5f6aa66db47dee",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b774a5652648e7bd23cdc02356237809dd6931e7 UNKNOWN
* 4419cf15c0320b96afcb3d767322721007a7268f Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445)
* 345706a3f30eea79d24f0bdb7c5f6aa66db47dee UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wangyang0918 commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425763495
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -300,19 +310,22 @@ public FileVisitResult visitFile(java.nio.file.Path file, BasicFileAttributes at
final ArrayList<String> classPaths = new ArrayList<>();
providedSharedLibs.forEach(
- (fileName, fileStatus) -> {
- localResources.put(
- fileName,
- Utils.registerLocalResource(
- fileStatus.getPath(),
- fileStatus.getLen(),
- fileStatus.getModificationTime(),
- LocalResourceVisibility.PUBLIC));
-
- if (!isFlinkDistJar(fileName)) {
- classPaths.add(fileName);
- }
- });
+ (fileName, fileStatus) -> {
+ final Path filePath = fileStatus.getPath();
+ LOG.debug("Using remote file {} to register local resource", filePath);
+
+ final YarnLocalResourceDescriptor descriptor = YarnLocalResourceDescriptor
+ .fromFileStatus(fileName, fileStatus, LocalResourceVisibility.PUBLIC);
+ localResources.put(fileName, descriptor.toLocalResource());
+ remotePaths.add(filePath);
+ envShipResourceList.add(descriptor);
+
+ if (!isFlinkDistJar(fileName) && !isPlugin(fileStatus.getPath())) {
Review comment:
I think we could not use `fileName` to check `isFlinkDistJar ` since it is relative path, not the real file name(e.g. lib/flink-dist_2.11-1.11-SNAPSHOT.jar). So i suggest to use the `key` or `relative` to instead of `fileName` to avoid confusing.
`fileStatus.getPath()` could be replaced with `filePath`.
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -367,6 +380,18 @@ private static boolean isFlinkDistJar(String fileName) {
return fileName.startsWith("flink-dist") && fileName.endsWith("jar");
}
+ private static boolean isPlugin(Path path) {
+ Path parent = path.getParent();
+ while (parent != null) {
+ if (parent.getName().equals("plugins")) {
Review comment:
"plugins" could be replaced with `ConfigConstants.DEFAULT_FLINK_PLUGINS_DIRS`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218834
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"triggerType" : "PUSH"
}, {
"hash" : "4419cf15c0320b96afcb3d767322721007a7268f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4419cf15c0320b96afcb3d767322721007a7268f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b774a5652648e7bd23cdc02356237809dd6931e7 UNKNOWN
* 4419cf15c0320b96afcb3d767322721007a7268f UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wangyang0918 commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629283389
Yes, all the tests run with a `OSSFileSystem` plugin.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629266502
Thanks a lot @wangyang0918 for the testing! Much appreciated! I will wait for Azure and then merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u closed pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u closed pull request #12170:
URL: https://github.com/apache/flink/pull/12170
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629270013
@wangyang0918 did you also try with a plugin, like a FileSystem?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425773599
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -367,6 +380,18 @@ private static boolean isFlinkDistJar(String fileName) {
return fileName.startsWith("flink-dist") && fileName.endsWith("jar");
}
+ private static boolean isPlugin(Path path) {
+ Path parent = path.getParent();
+ while (parent != null) {
+ if (parent.getName().equals("plugins")) {
Review comment:
Nice suggestions!!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wangyang0918 commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425778753
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -300,19 +310,22 @@ public FileVisitResult visitFile(java.nio.file.Path file, BasicFileAttributes at
final ArrayList<String> classPaths = new ArrayList<>();
providedSharedLibs.forEach(
- (fileName, fileStatus) -> {
- localResources.put(
- fileName,
- Utils.registerLocalResource(
- fileStatus.getPath(),
- fileStatus.getLen(),
- fileStatus.getModificationTime(),
- LocalResourceVisibility.PUBLIC));
-
- if (!isFlinkDistJar(fileName)) {
- classPaths.add(fileName);
- }
- });
+ (fileName, fileStatus) -> {
+ final Path filePath = fileStatus.getPath();
+ LOG.debug("Using remote file {} to register local resource", filePath);
+
+ final YarnLocalResourceDescriptor descriptor = YarnLocalResourceDescriptor
+ .fromFileStatus(fileName, fileStatus, LocalResourceVisibility.PUBLIC);
+ localResources.put(fileName, descriptor.toLocalResource());
+ remotePaths.add(filePath);
+ envShipResourceList.add(descriptor);
+
+ if (!isFlinkDistJar(fileName) && !isPlugin(fileStatus.getPath())) {
Review comment:
Yes, i think it could not work here.
```
private static boolean isFlinkDistJar(String fileName) {
return fileName.startsWith("flink-dist") && fileName.endsWith("jar");
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218834
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"triggerType" : "PUSH"
}, {
"hash" : "4419cf15c0320b96afcb3d767322721007a7268f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445",
"triggerID" : "4419cf15c0320b96afcb3d767322721007a7268f",
"triggerType" : "PUSH"
}, {
"hash" : "345706a3f30eea79d24f0bdb7c5f6aa66db47dee",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1432",
"triggerID" : "345706a3f30eea79d24f0bdb7c5f6aa66db47dee",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b774a5652648e7bd23cdc02356237809dd6931e7 UNKNOWN
* 4419cf15c0320b96afcb3d767322721007a7268f Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1445)
* 345706a3f30eea79d24f0bdb7c5f6aa66db47dee Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=1432)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425777590
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -300,19 +310,22 @@ public FileVisitResult visitFile(java.nio.file.Path file, BasicFileAttributes at
final ArrayList<String> classPaths = new ArrayList<>();
providedSharedLibs.forEach(
- (fileName, fileStatus) -> {
- localResources.put(
- fileName,
- Utils.registerLocalResource(
- fileStatus.getPath(),
- fileStatus.getLen(),
- fileStatus.getModificationTime(),
- LocalResourceVisibility.PUBLIC));
-
- if (!isFlinkDistJar(fileName)) {
- classPaths.add(fileName);
- }
- });
+ (fileName, fileStatus) -> {
+ final Path filePath = fileStatus.getPath();
+ LOG.debug("Using remote file {} to register local resource", filePath);
+
+ final YarnLocalResourceDescriptor descriptor = YarnLocalResourceDescriptor
+ .fromFileStatus(fileName, fileStatus, LocalResourceVisibility.PUBLIC);
+ localResources.put(fileName, descriptor.toLocalResource());
+ remotePaths.add(filePath);
+ envShipResourceList.add(descriptor);
+
+ if (!isFlinkDistJar(fileName) && !isPlugin(fileStatus.getPath())) {
Review comment:
You mean use the `filePath.getName()`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218834
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "b774a5652648e7bd23cdc02356237809dd6931e7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* b774a5652648e7bd23cdc02356237809dd6931e7 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wangyang0918 edited a comment on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
wangyang0918 edited a comment on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629283389
Yes, all the tests run with a `OSSFileSystem` plugin. I post one of the container directory here.
```
5629954 4 drwxr-s--- 6 yarn hadoop 4096 May 15 22:06 .
5629956 12 -rwx------ 1 yarn hadoop 11730 May 15 22:06 ./launch_container.sh
5629970 4 drwxr-s--- 3 yarn hadoop 4096 May 15 22:06 ./plugins
5629971 4 drwxr-s--- 2 yarn hadoop 4096 May 15 22:06 ./plugins/oss
1159174 15776 -r-xr-xr-x 1 hadoop hadoop 16150680 May 15 19:05 ./plugins/oss/flink-oss-fs-hadoop-1.11-SNAPSHOT.jar
4608003 4 -r-xr-xr-x 1 hadoop hadoop 654 May 8 21:11 ./plugins/README.txt
5629955 4 drwxr-s--- 2 yarn hadoop 4096 May 15 22:06 ./tmp
7307269 4 -r-x------ 1 yarn hadoop 1422 May 15 22:06 ./flink-conf.yaml
5629957 4 -rw------- 1 yarn hadoop 7 May 15 22:06 ./container_tokens
5629963 4 drwxr-s--- 2 yarn hadoop 4096 May 15 22:06 ./hello
2355206 4 -r-x------ 1 yarn hadoop 6 May 15 22:06 ./hello/111
4659203 4 -r-x------ 1 yarn hadoop 2101 May 15 22:06 ./log4j.properties
5629958 4 drwxr-s--- 2 yarn hadoop 4096 May 15 22:06 ./lib
2304013 272 -r-xr-xr-x 1 hadoop hadoop 276771 May 8 21:08 ./lib/log4j-api-2.12.1.jar
2304009 68 -r-xr-xr-x 1 hadoop hadoop 67114 May 8 21:08 ./lib/log4j-1.2-api-2.12.1.jar
3461129 7488 -r-xr-xr-x 1 hadoop hadoop 7667220 May 8 21:08 ./lib/flink-shaded-zookeeper-3.4.10.jar
231433 36040 -r-xr-xr-x 1 hadoop hadoop 36902985 May 8 21:08 ./lib/flink-table-blink_2.11-1.11-SNAPSHOT.jar
2304006 32352 -r-xr-xr-x 1 hadoop hadoop 33127498 May 8 21:08 ./lib/flink-table_2.11-1.11-SNAPSHOT.jar
137219 42304 -r-xr-xr-x 1 hadoop hadoop 43317025 May 8 21:08 ./lib/flink-shaded-hadoop-2-uber-2.8.3-10.0.jar
3483657 24 -r-xr-xr-x 1 hadoop hadoop 23518 May 8 21:08 ./lib/log4j-slf4j-impl-2.12.1.jar
6567939 1636 -r-xr-xr-x 1 hadoop hadoop 1674433 May 8 21:08 ./lib/log4j-core-2.12.1.jar
3485699 104068 -r-xr-xr-x 1 hadoop hadoop 106558347 May 15 21:04 ./lib/flink-dist_2.11-1.11-SNAPSHOT.jar
6748165 2828 -r-x------ 1 yarn hadoop 2892603 May 15 22:06 ./StateMachineExample.jar
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629238529
Thanks a lot @wangyang0918 ! Let me know about the results of your test and if Azure and you report positive results, then I will merge!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629201539
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit a213cea2e5a3d63f5ad88696a8d0befac1c061c0 (Fri May 15 12:14:58 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
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 <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629218223
I integrated your comments @wangyang0918 . Let me know what you think
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425777590
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -300,19 +310,22 @@ public FileVisitResult visitFile(java.nio.file.Path file, BasicFileAttributes at
final ArrayList<String> classPaths = new ArrayList<>();
providedSharedLibs.forEach(
- (fileName, fileStatus) -> {
- localResources.put(
- fileName,
- Utils.registerLocalResource(
- fileStatus.getPath(),
- fileStatus.getLen(),
- fileStatus.getModificationTime(),
- LocalResourceVisibility.PUBLIC));
-
- if (!isFlinkDistJar(fileName)) {
- classPaths.add(fileName);
- }
- });
+ (fileName, fileStatus) -> {
+ final Path filePath = fileStatus.getPath();
+ LOG.debug("Using remote file {} to register local resource", filePath);
+
+ final YarnLocalResourceDescriptor descriptor = YarnLocalResourceDescriptor
+ .fromFileStatus(fileName, fileStatus, LocalResourceVisibility.PUBLIC);
+ localResources.put(fileName, descriptor.toLocalResource());
+ remotePaths.add(filePath);
+ envShipResourceList.add(descriptor);
+
+ if (!isFlinkDistJar(fileName) && !isPlugin(fileStatus.getPath())) {
Review comment:
You mean use the `!isFlinkDistJar(filePath.getName())`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] wangyang0918 commented on pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on pull request #12170:
URL: https://github.com/apache/flink/pull/12170#issuecomment-629263865
@kl0u All the tests have passed. I am not aware of other issues. +1 for merging when azure passes.
* The provided lib works as expected
* Avoid unnecessary uploading and downloading(via public cache).
* Exception will be thrown when `flink-dist` is not included.
* The remote user jar works as expected.
* The old ship way works as expected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kl0u commented on a change in pull request #12170: [FLINK-17729] Make mandatory to have lib/, plugin/ and dist in yarn.provided.lib.dirs
Posted by GitBox <gi...@apache.org>.
kl0u commented on a change in pull request #12170:
URL: https://github.com/apache/flink/pull/12170#discussion_r425779943
##########
File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -300,19 +310,22 @@ public FileVisitResult visitFile(java.nio.file.Path file, BasicFileAttributes at
final ArrayList<String> classPaths = new ArrayList<>();
providedSharedLibs.forEach(
- (fileName, fileStatus) -> {
- localResources.put(
- fileName,
- Utils.registerLocalResource(
- fileStatus.getPath(),
- fileStatus.getLen(),
- fileStatus.getModificationTime(),
- LocalResourceVisibility.PUBLIC));
-
- if (!isFlinkDistJar(fileName)) {
- classPaths.add(fileName);
- }
- });
+ (fileName, fileStatus) -> {
+ final Path filePath = fileStatus.getPath();
+ LOG.debug("Using remote file {} to register local resource", filePath);
+
+ final YarnLocalResourceDescriptor descriptor = YarnLocalResourceDescriptor
+ .fromFileStatus(fileName, fileStatus, LocalResourceVisibility.PUBLIC);
+ localResources.put(fileName, descriptor.toLocalResource());
+ remotePaths.add(filePath);
+ envShipResourceList.add(descriptor);
+
+ if (!isFlinkDistJar(fileName) && !isPlugin(fileStatus.getPath())) {
Review comment:
yes, you are right, I already updated it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org