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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
