wangyang0918 commented on a change in pull request #18531:
URL: https://github.com/apache/flink/pull/18531#discussion_r802294778
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -918,6 +915,18 @@ private ApplicationReport startAppMaster(
: Path.CUR_DIR,
LocalResourceType.FILE);
+ // usrlib will be automatically shipped if it exists.
+ final Set<File> usrLibShipFiles = new HashSet<>();
Review comment:
Hmm. Maybe I did not make myself clear. I do not mean to remove the
condition check.
```
if (ClusterEntrypointUtils.tryFindUserLibDirectory().isPresent()) {
... ...
}
```
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnApplicationFileUploaderTest.java
##########
@@ -100,6 +105,35 @@ public void
testRegisterProvidedLocalResourcesWithDuplication() throws IOExcepti
}
}
+ @Test
+ public void testRegisterProvidedLocalResourcesWithNotAllowedUsrLib()
throws IOException {
+ final File flinkHomeDir = temporaryFolder.newFolder();
+ final File flinkLibDir = new File(flinkHomeDir, "lib");
+ final File flinkUsrLibDir = new File(flinkHomeDir, "usrlib");
+ final Map<String, String> libJars = getLibJars();
+ final Map<String, String> usrLibJars = getUsrLibJars();
+
+ generateFilesInDirectory(flinkLibDir, libJars);
+ generateFilesInDirectory(flinkUsrLibDir, usrLibJars);
+ List<Path> providedLibDirs = new ArrayList<>();
+ providedLibDirs.add(new Path(flinkLibDir.toURI()));
+ providedLibDirs.add(new Path(flinkUsrLibDir.toURI()));
+
+ assertThatThrownBy(
+ () ->
+ YarnApplicationFileUploader.from(
+ FileSystem.get(new
YarnConfiguration()),
+ new Path(flinkHomeDir.getPath()),
+ providedLibDirs,
+ ApplicationId.newInstance(0, 0),
+ DFSConfigKeys.DFS_REPLICATION_DEFAULT))
+ .isInstanceOf(RuntimeException.class)
Review comment:
Maybe we could use a more specific `IllegalArgumentException` here.
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##########
@@ -699,6 +701,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();
+ final Map<String, String> env = new HashMap<>(1);
+ File homeFolder = temporaryFolder.newFolder().getAbsoluteFile();
Review comment:
I believe `homeFolder` `libFolder` `usrLibFolder` `usrLibFile`
`effectiveShipFiles` could also be final.
##########
File path:
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java
##########
@@ -876,7 +874,6 @@ private ApplicationReport startAppMaster(
if (providedLibDirs == null || providedLibDirs.isEmpty()) {
addLibFoldersToShipFiles(systemShipFiles);
}
-
Review comment:
Unnecessary change.
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##########
@@ -699,6 +701,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();
+ final 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);
+ assertThat(effectiveShipFiles, containsInAnyOrder(usrLibFolder));
+ assertThat(effectiveShipFiles,
not(containsInAnyOrder(usrLibFile)));
Review comment:
I do not think we still need this assert. The above `containsInAnyOrder`
could cover such case.
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java
##########
@@ -699,6 +701,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();
+ final 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);
+ assertThat(effectiveShipFiles, containsInAnyOrder(usrLibFolder));
Review comment:
Please use `org.hamcrest.MatcherAssert.assertThat` instead.
--
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]
