bgeng777 commented on a change in pull request #18531:
URL: https://github.com/apache/flink/pull/18531#discussion_r802311817



##########
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:
       The `assertThat(effectiveShipFiles, 
not(containsInAnyOrder(usrLibFile)));` just follows the check logic in 
testEnvironmentDirectoryShipping to make sure single is not added.

##########
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:
       I remove this check because I double checked the codes and it should 
work well as we will do this check in addUsrLibFolderToShipFiles. 
   For readability and less object allocation/function call , I agree to get it 
back.




-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to