You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/10/20 09:05:52 UTC
[GitHub] [flink] reswqa opened a new pull request, #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
reswqa opened a new pull request, #21125:
URL: https://github.com/apache/flink/pull/21125
## What is the purpose of the change
*Flink AkkaRpcSystemLoader fails when temporary directory is a symlink. This is caused by FLINK-23500, as createDirectories will throw `FileAlreadyExistsException` if dir exists but is not a directory, such as symlink.*
## Brief change log
- *Support using symlink for `loadRpcSystem`.*
- *For symbolic link, create the linked dir if it doesn't exist.*
## Verifying this change
This change added tests in `AkkaRpcSystemLoaderITCase`.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000573557
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
Review Comment:
And the current commit cannot handle such a situation like '.../symlink/a/b', I will change the code to handle this.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21125:
URL: https://github.com/apache/flink/pull/21125#issuecomment-1285202566
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "8f8b29b90ddca1acfde860c3a751e284bd7c3f1f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "8f8b29b90ddca1acfde860c3a751e284bd7c3f1f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 8f8b29b90ddca1acfde860c3a751e284bd7c3f1f UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000571326
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
Review Comment:
@zentol After thinking for a while, I think it's OK not to deal with the situation where the symlink pointed to a non-exist file. What's your opinion?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on PR #21125:
URL: https://github.com/apache/flink/pull/21125#issuecomment-1293090715
@zentol , I have update this PR, please take a look again, thanks!
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000428552
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
+ }
Review Comment:
Make sense, i will move this logical to a file util.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1007918440
##########
flink-core/src/main/java/org/apache/flink/util/FileUtils.java:
##########
@@ -648,6 +648,29 @@ public static String stripFileExtension(String fileName) {
return fileName;
}
+ /**
+ * Get a target path(the path that replaced symbolic links with linked path) if the original
+ * path contains symbolic path, return the original path otherwise.
+ *
+ * @param path the original path.
+ * @return the path that replaced symbolic links with real path.
+ */
+ public static java.nio.file.Path getTargetPathIfContainsSymbolicPath(java.nio.file.Path path)
+ throws IOException {
+ java.nio.file.Path targetPath = path;
+ java.nio.file.Path suffixPath = Paths.get("");
+ while (path != null && path.getFileName() != null) {
+ if (Files.isSymbolicLink(path)) {
+ java.nio.file.Path linkedPath = path.toRealPath();
+ targetPath = Paths.get(linkedPath.toString(), suffixPath.toString());
+ break;
Review Comment:
I think it can be handled correctly, because java api(toRealPath) will replace all symbolic links in the path with real paths. But for security reasons, I added a test that includes multiple symbolic links.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000425937
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
Review Comment:
Do we _really_ have to handle the case where a symlink points to a non-existent file? How common is that?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] xintongsong closed pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
xintongsong closed pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
URL: https://github.com/apache/flink/pull/21125
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000434409
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
Review Comment:
To be honest, in the initial implementation, I did not cover the situation where symbolic links point to non-existent file. Note that the previous change(FLINK-23500) here is to create the temp directory if it does not exist. I follow the same specification for symbolic links, otherwise we will have two different behaviors. Although it may not be very common, symbolic links to non-existent files do exist.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1000425142
##########
flink-rpc/flink-rpc-akka-loader/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcSystemLoader.java:
##########
@@ -55,7 +55,13 @@ public RpcSystem loadRpcSystem(Configuration config) {
final ClassLoader flinkClassLoader = RpcSystem.class.getClassLoader();
final Path tmpDirectory = Paths.get(ConfigurationUtils.parseTempDirectories(config)[0]);
- Files.createDirectories(tmpDirectory);
+ if (!Files.isSymbolicLink(tmpDirectory)) {
+ // Create tmp dir if it doesn't exist.
+ Files.createDirectories(tmpDirectory);
+ } else if (!Files.exists(tmpDirectory)) {
+ // For symbolic link, create the linked dir if it doesn't exist.
+ Files.createDirectory(Files.readSymbolicLink(tmpDirectory));
+ }
Review Comment:
Logic like this doesn't belong in here, but in some re-usable file util.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] xintongsong commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
xintongsong commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1007846839
##########
flink-core/src/main/java/org/apache/flink/util/FileUtils.java:
##########
@@ -648,6 +648,29 @@ public static String stripFileExtension(String fileName) {
return fileName;
}
+ /**
+ * Get a target path(the path that replaced symbolic links with linked path) if the original
+ * path contains symbolic path, return the original path otherwise.
+ *
+ * @param path the original path.
+ * @return the path that replaced symbolic links with real path.
+ */
+ public static java.nio.file.Path getTargetPathIfContainsSymbolicPath(java.nio.file.Path path)
+ throws IOException {
+ java.nio.file.Path targetPath = path;
+ java.nio.file.Path suffixPath = Paths.get("");
+ while (path != null && path.getFileName() != null) {
+ if (Files.isSymbolicLink(path)) {
+ java.nio.file.Path linkedPath = path.toRealPath();
+ targetPath = Paths.get(linkedPath.toString(), suffixPath.toString());
+ break;
Review Comment:
What happens if there're multiple symbolic links in the path?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] reswqa commented on a diff in pull request #21125: [FLINK-28102] Flink AkkaRpcSystemLoader fails when temporary directory is a symlink
Posted by GitBox <gi...@apache.org>.
reswqa commented on code in PR #21125:
URL: https://github.com/apache/flink/pull/21125#discussion_r1007918440
##########
flink-core/src/main/java/org/apache/flink/util/FileUtils.java:
##########
@@ -648,6 +648,29 @@ public static String stripFileExtension(String fileName) {
return fileName;
}
+ /**
+ * Get a target path(the path that replaced symbolic links with linked path) if the original
+ * path contains symbolic path, return the original path otherwise.
+ *
+ * @param path the original path.
+ * @return the path that replaced symbolic links with real path.
+ */
+ public static java.nio.file.Path getTargetPathIfContainsSymbolicPath(java.nio.file.Path path)
+ throws IOException {
+ java.nio.file.Path targetPath = path;
+ java.nio.file.Path suffixPath = Paths.get("");
+ while (path != null && path.getFileName() != null) {
+ if (Files.isSymbolicLink(path)) {
+ java.nio.file.Path linkedPath = path.toRealPath();
+ targetPath = Paths.get(linkedPath.toString(), suffixPath.toString());
+ break;
Review Comment:
I think it can be handled correctly, because java api(toRealPath) will replace all symbolic links in the path with real paths. But for robustness, I added a test that includes multiple symbolic links.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org