wangyang0918 commented on a change in pull request #18531:
URL: https://github.com/apache/flink/pull/18531#discussion_r802223194
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -277,12 +278,9 @@ public void setLocalJarPath(Path localJarPath) {
*/
public void addShipFiles(List<File> shipFiles) {
checkArgument(
- userJarInclusion != YarnConfigOptions.UserJarInclusion.DISABLED
- || isUsrLibDirIncludedInShipFiles(shipFiles),
- "This is an illegal ship directory : %s. When setting the %s
to %s the name of ship directory can not be %s.",
+ !isUsrLibDirIncludedInShipFiles(shipFiles),
+ "This is an illegal ship directory : %s. %s can not be shipped
again as it will be automatically shipped if exists.",
Review comment:
```suggestion
"User shipped directories, configured via %s, should not
include %s.",
```
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -918,6 +915,22 @@ private ApplicationReport startAppMaster(
: Path.CUR_DIR,
LocalResourceType.FILE);
+ // usrlib will be automatically shipped if it exists.
+ final boolean isLocalUsrLibExists =
+ ClusterEntrypointUtils.tryFindUserLibDirectory().isPresent();
+ if (isLocalUsrLibExists) {
Review comment:
Maybe the local variable `isLocalUsrLibExists` is unnecessary.
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -1752,11 +1780,11 @@ ContainerLaunchContext setupApplicationMasterContainer(
return config.get(YarnConfigOptions.CLASSPATH_INCLUDE_USER_JAR);
}
- private static boolean isUsrLibDirIncludedInShipFiles(List<File>
shipFiles) {
+ private static boolean isUsrLibDirIncludedInShipFiles(Collection<File>
shipFiles) {
Review comment:
We do not need this change.
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java
##########
@@ -100,6 +100,11 @@ private YarnApplicationFileUploader(
this.localResources = new HashMap<>();
this.applicationDir = getApplicationDir(applicationId);
+ checkArgument(
+ !isUsrLibDirIncludedInProvidedLib(providedLibDirs),
+ "There is an illegal directory in %s : %s.",
+ YarnConfigOptions.PROVIDED_LIB_DIRS.key(),
+ ConfigConstants.DEFAULT_FLINK_USR_LIB_DIR);
Review comment:
```suggestion
checkArgument(
!isUsrLibDirIncludedInProvidedLib(providedLibDirs),
"Provided lib directories, configured via %s, should not
include %s.",
YarnConfigOptions.PROVIDED_LIB_DIRS.key(),
ConfigConstants.DEFAULT_FLINK_USR_LIB_DIR);
```
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -277,12 +278,9 @@ public void setLocalJarPath(Path localJarPath) {
*/
public void addShipFiles(List<File> shipFiles) {
checkArgument(
- userJarInclusion != YarnConfigOptions.UserJarInclusion.DISABLED
- || isUsrLibDirIncludedInShipFiles(shipFiles),
- "This is an illegal ship directory : %s. When setting the %s
to %s the name of ship directory can not be %s.",
+ !isUsrLibDirIncludedInShipFiles(shipFiles),
+ "This is an illegal ship directory : %s. %s can not be shipped
again as it will be automatically shipped if exists.",
ConfigConstants.DEFAULT_FLINK_USR_LIB_DIR,
Review comment:
```suggestion
YarnConfigOptions.SHIP_FILES.key(),
```
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##########
@@ -699,6 +699,32 @@ public void
testDisableSystemClassPathIncludeUserJarAndWithIllegalShipDirectoryN
}
}
+ /** Tests that the usrlib will be automatically shipped. */
+ @Test
+ public void testShipUsrLib() throws IOException {
+ final Map<String, String> oldEnv = System.getenv();
+ Map<String, String> env = new HashMap<>(1);
+ File homeFolder = temporaryFolder.newFolder().getAbsoluteFile();
+ File libFolder = new File(homeFolder.getAbsolutePath(), "lib");
+ assertTrue(libFolder.createNewFile());
+ File usrLibFolder =
+ new File(homeFolder.getAbsolutePath(),
ConfigConstants.DEFAULT_FLINK_USR_LIB_DIR);
+ assertTrue(usrLibFolder.mkdirs());
+ File usrLibFile = new File(usrLibFolder, "usrLibFile.jar");
+ assertTrue(usrLibFile.createNewFile());
+ env.put(ConfigConstants.ENV_FLINK_LIB_DIR,
libFolder.getAbsolutePath());
+ CommonTestUtils.setEnv(env);
+
+ try (YarnClusterDescriptor descriptor = createYarnClusterDescriptor())
{
+ Set<File> effectiveShipFiles = new HashSet<>();
+ descriptor.addUsrLibFolderToShipFiles(effectiveShipFiles);
+ Assert.assertFalse(effectiveShipFiles.contains(usrLibFile));
Review comment:
Use `assertThat` is always recommended.
```
assertThat(effectiveShipFiles, containsInAnyOrder(usrLibFolder));
```
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -918,6 +915,22 @@ private ApplicationReport startAppMaster(
: Path.CUR_DIR,
LocalResourceType.FILE);
+ // usrlib will be automatically shipped if it exists.
+ final boolean isLocalUsrLibExists =
+ ClusterEntrypointUtils.tryFindUserLibDirectory().isPresent();
+ if (isLocalUsrLibExists) {
+ Set<File> usrLibShipFiles = new HashSet<>();
Review comment:
`usrLibShipFiles` could be final.
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##########
@@ -699,6 +699,32 @@ public void
testDisableSystemClassPathIncludeUserJarAndWithIllegalShipDirectoryN
}
}
+ /** Tests that the usrlib will be automatically shipped. */
+ @Test
+ public void testShipUsrLib() throws IOException {
+ final Map<String, String> oldEnv = System.getenv();
+ Map<String, String> env = new HashMap<>(1);
Review comment:
Please use `final` if possible.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
