wangyang0918 commented on a change in pull request #18531: URL: https://github.com/apache/flink/pull/18531#discussion_r801282398
########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java ########## @@ -1685,6 +1699,35 @@ void addLibFoldersToShipFiles(Collection<File> effectiveShipFiles) { } } + @VisibleForTesting + void addUsrLibFolderToShipFiles( + Collection<File> effectiveShipFiles, Collection<File> systemShipFiles) { + // Add usrlib folder to the ship files if it exists + // Classes in the folder will be loaded by UserClassLoader if CLASSPATH_INCLUDE_USER_JAR is + // DISABLED. + final Optional<File> usrLibDir = getLocalUsrLibDirectory(); + + if (usrLibDir.isPresent()) { + File usrLibDirFile = usrLibDir.get(); + if (usrLibDirFile.isDirectory()) { + checkArgument( Review comment: Given that the shipped files could not include `usrlib`. So I think we could reuse the `checkArgument` in `YarnClusterDescriptor#addShipFiles`. And it need to be changed like following. WDYT? ``` checkArgument( !isUsrLibDirIncludedInShipFiles(shipFiles), "The name of ship directory can not be %s.", DEFAULT_FLINK_USR_LIB_DIR); ``` After then the implementation of `addUsrLibFolderToShipFiles` could be simplified. ``` @VisibleForTesting void addUsrLibFolderToShipFiles(Collection<File> effectiveShipFiles) { ClusterEntrypointUtils.tryFindUserLibDirectory() .ifPresent( usrLibDirFile -> { effectiveShipFiles.add(usrLibDirFile); LOG.info( "usrlib: {} will be shipped automatically.", usrLibDirFile.getAbsolutePath()); }); } ``` ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java ########## @@ -438,6 +446,18 @@ private static boolean isPlugin(Path path) { return false; } + private static boolean isUsrLib(Path path) { Review comment: I suggest to introduce the `isUsrLibDirIncludedInProvidedLib` instead and then `checkArgument` in the constructor of `YarnApplicationFileUploader`. Because the files in the provided lib will be relativized, so we just need to check the name of provided lib. ``` private boolean isUsrLibDirIncludedInProvidedLib(final List<Path> providedLibDirs) throws IOException { for (Path path : providedLibDirs) { final FileStatus fileStatus = fileSystem.getFileStatus(path); if (fileStatus.isDirectory() && ConfigConstants.DEFAULT_FLINK_USR_LIB_DIR.equals(path.getName())) { return true; } } return false; } ``` ########## File path: flink-yarn/pom.xml ########## @@ -130,6 +130,11 @@ under the License. </exclusion> </exclusions> </dependency> + <dependency> Review comment: Why do we need this change? I think we already have the dependency in the parent pom. ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java ########## @@ -1799,4 +1842,27 @@ public static void logDetachedClusterInformation( yarnApplicationId, yarnApplicationId); } + + /** Review comment: Could we use `ClusterEntrypointUtils.tryFindUserLibDirectory()` to avoid introducing the `getLocalUsrLibDirectory()`? ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java ########## @@ -910,14 +910,28 @@ private ApplicationReport startAppMaster( } // Upload and register user jars - final List<String> userClassPaths = + List<String> userClassPaths = Review comment: Why do we remove the `final`? ########## File path: flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java ########## @@ -699,6 +700,41 @@ public void testDisableSystemClassPathIncludeUserJarAndWithIllegalShipDirectoryN } } + /** + * Tests that the usrlib is added ship files again when local usrlib has been automatically + * shipped. + */ + @Test + public void testShipUsrLibWithExistingLocalUsrLib() throws IOException { Review comment: After moving out the `checkArgument` in `YarnClusterDescriptor#addUsrLibFolderToShipFiles`, we just need to test whether the `usrlib` is added to shipped files automatically. ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java ########## @@ -1752,7 +1795,7 @@ 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: I think we need to correct the implementation of `isUsrLibDirIncludedInShipFiles`. When it returns `true`, it should mean that the specified shipFiles include the `usrlib`. Currently, it has the opposite meaning. ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java ########## @@ -335,7 +335,7 @@ public YarnLocalResourceDescriptor uploadFlinkDist(final Path localJarPath) * * @return list of class paths with the file name */ - List<String> registerProvidedLocalResources() { + List<String> registerProvidedLocalResources(boolean shouldUsrLibExistInRemote) { Review comment: We assume that the `usrlib` should always not be included in provided lib whether `usrlib` exists locally or not. Right? It just have the same semantic with ship files. ########## File path: flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java ########## @@ -872,14 +872,14 @@ private ApplicationReport startAppMaster( jobGraph.writeUserArtifactEntriesToConfiguration(); } - + boolean isLocalUsrLibExists = getLocalUsrLibDirectory().isPresent(); Review comment: `isLocalUsrLibExists` could be `final`. -- 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